Repository navigation
Conversation
We're rolling out hot-reloading producers in Relay: https://app.notion.com/p/sentry/Unifying-config-layer-for-kafka-producer-consumer-config-3b58b10e4b5d80388c64c2964f1e5329 This PR introduces arroyo's hot-reloading producer into Relay. As a first step, config is still built from Relay's own kafka config, but works for live-reload. The new producer can be rolled out per-topic with `processing.reloading_producer_topics`. There shouldn't be any behavioral changes besides the fact that hot-reloading producer settings should now work. Once reloading-producer is fully rolled out along all topics, we can swap out relay's kafka config with a single centralized one (the "blob"). What doesn't work is renaming topics live. Relay keys topics by physical topic name in a few places, and arroyo abstracts the physical topic name away insufficiently. This will be fixed in a followup, probably around the time we introduce slicing/sharding into arroyo. Depends on getsentry/arroyo#578, pinned by branch until it is released.
| /// Topics using the reloading producer take their new broker settings from here; see | ||
| /// `processing.reloading_producer_topics`. Everything else picks them up on restart. | ||
| #[cfg(feature = "processing")] | ||
| kafka_client: Option<Arc<KafkaClient>>, |
There was a problem hiding this comment.
if this arc is not desired, ConfigReloadService could send messages to StoreService for reload
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6affe4f. Configure here.
# Conflicts: # CHANGELOG.md # Cargo.lock # Cargo.toml
The merge left the entry under 26.10.0, which master had already released, and it was missing the PR link. Also apply black.
They set `processing.enabled`, which a build without the feature rejects, so they failed on the macOS and Windows jobs that run without it.
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
LGTM - please give others also a chance to take a look before merging
| /// | ||
| /// The blob is meant to be opaque to Relay: options-automator publishes one per region | ||
| /// covering every cluster and topic, and Relay passes the bytes to Arroyo without | ||
| /// interpreting them. That delivery channel does not exist, so this builds a blob out of |
There was a problem hiding this comment.
That delivery channel does not exist - "yet" - I assume that's one of your next steps?
| // left the Kafka config alone. Arroyo compares the resolved config and | ||
| // ignores a push that changes nothing, so a redundant one is harmless. | ||
| #[cfg(feature = "processing")] | ||
| self.push_kafka_config(); |
There was a problem hiding this comment.
I think this works for now and can stay. Though maybe in the future we may want a more generic mechanism if we need such a thing in more places in Relay.
One idea is to keep track of the config generation in relay-config and the producer can keep track of the current generation, whenever it observes a change it can initiate a reload, this would get away from the push model more into a polling model.
In the end we still want some kind of de-duplicate of configs in the producer to not reload when other parts of the config changed.
| /// | ||
| /// The blob is meant to be opaque to Relay: options-automator publishes one per region | ||
| /// covering every cluster and topic, and Relay passes the bytes to Arroyo without | ||
| /// interpreting them. That delivery channel does not exist, so this builds a blob out of |
There was a problem hiding this comment.
yes
| /// interpreting them. That delivery channel does not exist, so this builds a blob out of | |
| /// interpreting them. That delivery channel does not yet exist, so this builds a blob out of |
(note to self: gotta apply this with proper line-wrapping before merging and also trim down some of the sloppy paragraphs below)
This branch has not been deployed
2 participants
We're rolling out hot-reloading producers in Relay: https://app.notion.com/p/sentry/Unifying-config-layer-for-kafka-producer-consumer-config-3b58b10e4b5d80388c64c2964f1e5329
This PR introduces arroyo's hot-reloading producer into Relay. As a first step, config is still built from Relay's own kafka config, but works for live-reload. The new producer can be rolled out per-topic with
processing.reloading_producer_topics. There shouldn't be any behavioral changes besides the fact that hot-reloading producer settings should now work.Once reloading-producer is fully rolled out along all topics, we can swap out relay's kafka config with a single centralized one (the "blob").
What doesn't work is renaming topics live. Relay keys topics by physical topic name in a few places, and arroyo abstracts the physical topic name away insufficiently. This will be fixed in a followup, probably around the time we introduce slicing/sharding into arroyo.
Depends on getsentry/arroyo#578, pinned by branch until it is released.
ref STREAM-1834