New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ref: Remove all references to settings.KAFKA_TOPICS so it can be removed #66288
Conversation
…ides This moves the `dlq_topic` consumer definitions and anything that uses the `get_topic_definition` helper function over to the new style kafka topic config.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #66288 +/- ##
=======================================
Coverage 84.30% 84.30%
=======================================
Files 5311 5311
Lines 237263 237258 -5
Branches 41036 41035 -1
=======================================
- Hits 200016 200015 -1
+ Misses 37028 37024 -4
Partials 219 219
|
kafka_topic_dict = settings.KAFKA_TOPICS[kafka_topic_name] | ||
assert kafka_topic_dict is not None | ||
cluster_name = kafka_topic_dict["cluster"] | ||
logical_topic = Topic.INGEST_PERFORMANCE_METRICS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this Topic
enum a basic class with strings? some of the tests fail now because an enum variant is not an actual string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, the failures are a real bug. Looks like there are more pre-requisites before I can merge this (specifically, all the consumer definitions need to use the new way)
Depends on #66283 and #66343