-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat(operator): static membership #346
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
base: bmcquilkin/operator/pods
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
| build_kafka_producer_config, | ||
| ) | ||
| from sentry_streams.adapters.stream_adapter import RuntimeTranslator | ||
| from sentry_streams.config_types import StepConfig | ||
| from sentry_streams.config_types import KafkaConsumerConfig, StepConfig | ||
| from sentry_streams.pipeline.pipeline import Pipeline | ||
| from sentry_streams.runner import iterate_edges | ||
| from sentry_streams.rust_streams import InitialOffset | ||
|
|
@@ -46,6 +46,7 @@ def test_rust_arroyo_adapter( | |
| [ | ||
| pytest.param( | ||
| { | ||
| "starts_segment": None, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this ? |
||
| "bootstrap_servers": ["localhost:9092"], | ||
| "auto_offset_reset": "earliest", | ||
| }, | ||
|
|
@@ -59,6 +60,7 @@ def test_rust_arroyo_adapter( | |
| ), | ||
| pytest.param( | ||
| { | ||
| "starts_segment": None, | ||
| "bootstrap_servers": ["localhost:9092"], | ||
| "auto_offset_reset": "latest", | ||
| "consumer_group": "my-group", | ||
|
|
@@ -75,6 +77,7 @@ def test_rust_arroyo_adapter( | |
| ), | ||
| pytest.param( | ||
| { | ||
| "starts_segment": None, | ||
| "bootstrap_servers": ["broker1:9092", "broker2:9092"], | ||
| "auto_offset_reset": "earliest", | ||
| "consumer_group": "config-group", | ||
|
|
@@ -94,7 +97,7 @@ def test_rust_arroyo_adapter( | |
| ], | ||
| ) | ||
| def test_build_kafka_consumer_config( | ||
| source_config: StepConfig, | ||
| source_config: KafkaConsumerConfig, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests ignore static membership envLow Severity
Reviewed by Cursor Bugbot for commit cfec667. Configure here. |
||
| consumer_group_override: str | None, | ||
| expected_bootstrap_servers: Sequence[str], | ||
| expected_group_id: str, | ||
|
|
@@ -157,3 +160,54 @@ def test_build_kafka_producer_config( | |
| assert result is not None | ||
| assert list(result.bootstrap_servers) == expected_bootstrap_servers | ||
| assert result.override_params == expected_override_params | ||
|
|
||
|
|
||
| def test_build_kafka_consumer_config_injects_static_membership( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| monkeypatch.setenv("STREAMS_KAFKA_GROUP_INSTANCE_ID", "consumer-2") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You do not need to monkeypatch environment variables. Just set that with In general please try to avoid monkey patching whenever it is not strictly necessary. It is really hard to makes sense of the behavior of the class you are patching when it evolves. |
||
| result = build_kafka_consumer_config( | ||
| source="test_source", | ||
| source_config={ | ||
| "starts_segment": None, | ||
| "bootstrap_servers": ["localhost:9092"], | ||
| "auto_offset_reset": "earliest", | ||
| }, | ||
| ) | ||
| assert result.override_params is not None | ||
| assert result.override_params["group.instance.id"] == "consumer-2" | ||
|
|
||
|
|
||
| def test_build_kafka_consumer_config_respects_explicit_instance_id( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| monkeypatch.setenv("STREAMS_KAFKA_GROUP_INSTANCE_ID", "consumer-2") | ||
| result = build_kafka_consumer_config( | ||
| source="test_source", | ||
| source_config={ | ||
| "starts_segment": None, | ||
| "bootstrap_servers": ["localhost:9092"], | ||
| "auto_offset_reset": "earliest", | ||
| "override_params": { | ||
| "group.instance.id": "explicit", | ||
| "session.timeout.ms": "10000", | ||
| }, | ||
| }, | ||
| ) | ||
| assert result.override_params is not None | ||
| assert result.override_params["group.instance.id"] == "explicit" | ||
|
|
||
|
|
||
| def test_build_kafka_consumer_config_no_static_membership_without_env( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| monkeypatch.delenv("STREAMS_KAFKA_GROUP_INSTANCE_ID", raising=False) | ||
| result = build_kafka_consumer_config( | ||
| source="test_source", | ||
| source_config={ | ||
| "starts_segment": None, | ||
| "bootstrap_servers": ["localhost:9092"], | ||
| "auto_offset_reset": "earliest", | ||
| }, | ||
| ) | ||
| assert result.override_params == {} | ||


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.
You do not need to read environment variables from inside the adapter. The code that reads the config file is already able to transparently swap a config field with an environment variable provided according to a standard format
https://github.com/getsentry/streams/blob/main/sentry_streams/sentry_streams/pipeline/config.py#L26-L32
See an example of how this is used
This means you can leverage the standard override mechanism without doing anything special for group.instance.id here.
More in general, beware of reading random environment variables deep in the stack. That makes it very hard to track down how to configure your system.
A better option is to have a config abstraction that provide the whole configuration and takes care to resolve, config files, overrides with environment variables and other ways to configure the system.
Then you separate the concerns cleanly. One system manages the loading of the config and another consumes it. The system consuming the config does not have to care where the value comes from.