refactor(ingestion): Make KAFKA_ENABLED true by default and set KAFKA_HOSTS default
#9844
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KAFKA_ENABLED true by defaultKAFKA_ENABLED true by default and set KAFKA_HOSTS default
tiina303
approved these changes
May 19, 2022
| @@ -136,9 +136,6 @@ export async function createHub( | |||
|
|
|||
| if (serverConfig.KAFKA_ENABLED) { | |||
There was a problem hiding this comment.
I assume we'll do a follow-up where we remove the variable completely?
There was a problem hiding this comment.
Yup! Whenever we migrate another suite to run the ClickHouse pipeline, this should be deleted from serverConfigs, and soon should be used nowhere.
| @@ -25,6 +25,7 @@ async function createTestHub(): Promise<[Hub, () => Promise<void>]> { | |||
| PLUGINS_CELERY_QUEUE: 'ttt-test-plugins-celery-queue', | |||
| CELERY_DEFAULT_QUEUE: 'ttt-test-celery-default-queue', | |||
| LOG_LEVEL: LogLevel.Log, | |||
| KAFKA_ENABLED: false, | |||
There was a problem hiding this comment.
I updated this test in my redisqueue removal pr, so whatever makes these tests pass here is fine and will be ignored 😅
| # URL Used by kafka clients/producers | ||
| KAFKA_URL = os.getenv("KAFKA_URL", "kafka://kafka") | ||
| # URL(s) used by Kafka clients/producers - KEEP IN SYNC WITH plugin-server/src/config/config.ts | ||
| KAFKA_URL = os.getenv("KAFKA_URL", "kafka://kafka:9092") |
There was a problem hiding this comment.
why are there these two places?
There was a problem hiding this comment.
Before this PR I just merged KAFKA_HOSTS in Python had a default, but it was a bit off because it didn't have the port, which is usually required. While the plugin server required KAFKA_HOSTS to be provided explicitly. Now this is 100% synced, for better development ergonomics (no need to set KAFKA_HOSTS anywhere in dev) and a more obvious relation between those codebases.
fuziontech
added a commit
that referenced
this pull request
May 19, 2022
* master: refactor(ingestion): Make `KAFKA_ENABLED` true by default and set `KAFKA_HOSTS` default (#9844) feat(apps): transpile frontend.tsx (#9828) feat: show api call status when adding insights to dashboards (#9817) feat: track metrics on zapier hook firings (#9866) fix(onboarding): instrumentation (#9845) feat(whitelabel-shared-dashboard): Hide branding on shared dashboards paid (#9849) fix(apps): plugin source quickfix (#9862) refactor(plugin-server): Remove `ts-jest`, use `jest.mocked` (#9861) refactor(plugin-server): Trigger public jobs with graphile insted of celery queue (#9811) chore: upgrade pip tools (#9859) feat(apps): plugin source in its own model, part 2 (#9854) chore: Update sprint_planning_retro.md (#9791) feat(apps): plugin source in its own model, part 1 (#9853) feat(matrix): Add option to save `simulate_matrix` like `setup_dev` (#9836) fix(cohort): add mapping from event to person (#9841) feat(person-on-events): Enable CI to run using both old and new queries (#9814)
alexkim205
pushed a commit
that referenced
this pull request
May 23, 2022
…FKA_HOSTS` default (#9844) * refactor(ingestion): Make `KAFKA_ENABLED` true by default * Sync `KAFKA_HOSTS` defaults too * Update snapshots * Update "kafka" to "kafka:9092" * Revert "Update "kafka" to "kafka:9092"" This reverts commit d954ac6. * Update some tests * Revert "Revert "Update "kafka" to "kafka:9092""" This reverts commit 07edfa6. * Update test_0004_replicated_schema.ambr * Remove `KAFKA_ENABLED` and `KAFKA_HOSTS` from places Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The
KAFKA_ENABLEDsetting hasn't made sense for a long time, since PostHog does not work withKAFKA_ENABLEDother thantrueanymore. Getting rid of this setting is important on the way to removing useless code of Postgres-based ingestion (yeetcode).Changes
For now just trying to see what breaks when the default is toggled.