feat: attach the rebalance listener to concurrent subscribers automatically - #87
Merged
Merged
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
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
ConsumerRebalanceListeneris the only thing that commits finished work when partitions are revoked, but nothing attached it:listener=; otherwise it installs just its own logging listener (faststream/kafka/subscriber/usecase.py).handler.create_rebalance_listener()needs theKafkaConcurrentHandler, which only exists once the lifespan has run, while subscribers (often on routers) are declared at build time. The README quick start passed no listener and called it "optional".So services ran without one. Every rebalance (e.g. each rolling deploy) skipped the flush, finished work on revoked partitions was redelivered to the new owner, and late commits for those partitions failed with
IllegalStateError(see #86).Change
Automatic attachment.
initialize_concurrent_processingnow attaches a listener to every subscriber this library processes concurrently:context.get("app").brokers→broker.subscribers, which includes subscribers from included routers); the app registers itself inbroker.contextfor bothFastStreamandAsgiFastStream;LogicSubscriber,AckPolicy.MANUAL, notbatch=True, subscribed by topic or pattern (manually assigned partitions never rebalance);listener=is kept and called after the flush (sync or async, as aiokafka allows); an explicitConsumerRebalanceListeneris left untouched; a secondinitializedoes not wrap twice.New parameter
rebalance_flush_timeout_sec(default 10 s) configures the attached listener's flush budget.Logged at ERROR, once per
initialize, when it cannot attach: the broker has already started (the listener is fixed atconsumer.subscribe()), or the context holds no FastStream application. Both leave a subscriber that needs the flush without one, so every rebalance redelivers finished work until the setup is fixed; that is a misconfiguration an error reporter should surface. Subscribers that do not need a listener are skipped silently.Explicit fallback.
ConsumerRebalanceListener.from_context(context, flush_timeout_sec=...)builds the same context-resolving listener for setups where attachment is not possible.KafkaConcurrentHandler.committeris a new read-only property it uses. The existing constructor andhandler.create_rebalance_listener()are unchanged.The private attribute
FastStream has no public way to add a rebalance listener after a subscriber is declared, so attachment writes
subscriber._listener, which FastStream reads when the consumer subscribes. The coupling is bounded:<0.8, and the floors CI leg runs 0.7.1;LogicSubscriber.startorConcurrentBetweenPartitionsSubscriber.startstops passinglistener=self._listener;Setting a listener after declaration is not possible through any public FastStream API, and the alternative (every subscriber passing
listener=) is what services were not doing.Tests
KafkaBroker/KafkaRouter/FastStream, no network; failed before the change): attached to MANUAL subscribers on the broker and on an included router with the configured timeout; attached listener flushes the running handler; not attached to non-MANUAL, batch, or partition-assigned subscribers; user listener kept and called (sync and async); explicit listener left alone; no double wrap after restart; ERROR when no app; ERROR naming the topic when the broker is already running; FastStream guard test.from_contextunit tests: handler registered after the listener is resolved; no handler → no-op; stopped handler → no flush.FastStreamapp, nolistener=) processes a message with commit batch size/timeout out of reach, then a second member joins the group; the offset must be committed by the revoke callback. With theattach_rebalance_listenerscall removed the test fails ({} == {0: 1}).just test): 211 passed, 100% coverage. Unit suite also passes on the floors (FastStream 0.7.1, aiokafka 0.14.0). ruff and ty clean.