ref: Rationalize kafka config - take 2 #66242
Conversation
This reverts commit 369b089.
github-actions
bot
added
the
Scope: Backend
src/sentry/consumers/__init__.py
Outdated
| from arroyo.backends.kafka.configuration import build_kafka_consumer_configuration | ||
| from arroyo.backends.kafka.consumer import KafkaConsumer | ||
| from arroyo.commit import ONCE_PER_SECOND | ||
| from arroyo.types import Topic as ArroyoTopic | ||
| from django.conf import settings | ||
|
|
||
| from sentry.utils import kafka_config |
There was a problem hiding this comment.
a lot of these imports can be removed entirely -- or moved to the top of the file (some are already there)
There was a problem hiding this comment.
moved up the arroyo imports, gonna leave the rest as-is
| SNUBA_SPANS = "snuba-spans" | ||
|
|
||
|
|
||
| class ConsumerDefinition(TypedDict, total=False): |
There was a problem hiding this comment.
is this actually supposed to be total=False? all keys are optional?
I usually find for dicts where the are some required keys that it's easier to understand if the not-require ones are marked NotRequired rather than total=False + Required
There was a problem hiding this comment.
i'm not sure, i'm somewhat reluctant to touch this code in this PR. it's just copy + paste
| dlq_max_invalid_ratio: float | None | ||
| dlq_max_consecutive_count: int | None |
There was a problem hiding this comment.
are these actually nullable or are they meant to be NotRequired
There was a problem hiding this comment.
I think nullable is right here
| def validate_consumer_definition(consumer_definition: ConsumerDefinition) -> None: | ||
| if "dlq_topic" not in consumer_definition and ( | ||
| "dlq_max_invalid_ratio" in consumer_definition | ||
| or "dlq_max_consecutive_count" in consumer_definition | ||
| ): | ||
| raise ValueError( | ||
| "Invalid consumer definition, dlq_max_invalid_ratio/dlq_max_consecutive_count is configured, but dlq_topic is not" | ||
| ) |
There was a problem hiding this comment.
hmm, the intention of the types modules is that they are just types -- this kind of goes against it by having logic here
There was a problem hiding this comment.
This is just copy + paste
Redo of #65793 with fixed imports