feat: add graceful streaming pull shutdown #292
Merged
Conversation
This file contains 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
This internal flag has been hardcoded to True for several years and we do not utilize the "False" case anywhere, nor do we mention it in the docs or expose it to the end users.
The leaser thread should not terminate implicitly when the streaming pull manager's consumer thread becomes inactive, as there might still be messages being processed inside the scheduler and we want to keep extending these messages' ACK deadlines until the scheduler has been shut down. NOTE: The manager's streaming pull RPC does not need to be active, since all mod-ACK requests are sent using a separate unary request instead of over the stream.
The library only supports Python 3.6+, thus we don't need the conditional anymore.
For consistency with the leaser, the heartbeater should not terminate implicitly when the manager stops the background stream, but should instead wait for the manager to stop it explicitly.
After the streaming pull manager shuts down the consumer thread and becomes "inactive", there might still be requests waiting in the queue to be dispatched, thus the dispatcher should not implicitly enter the no-op mode of operation.
product-auto-label
bot
added
the
api: pubsub
google-cla
bot
added
the
cla: yes
plamut
changed the title
plamut
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
|
It appears that ACK requests sent after the shutdown somehow get lost, even though the dispatcher processes them and sends them through the streaming pull manager's ( Update: This was because all messages were published in the same batch, meaning that all of them had to be ACK-ed for the ACKs to persist. It's the backend behavior that will be changed, but not in the immediate future. |
kamalaboulhosn
approved these changes
Feb 17, 2021
|
This trickled down to |
anguillanneuf
added a commit
that referenced
this pull request
Mar 10, 2021
This reverts commit 00874fe.
This was referenced Mar 10, 2021
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.
Closes #276.
This PR implements an optional blocking streaming pull shutdown. "Blocking" means blocking on
streaming_future.cancel()until all currently executing user callbacks complete, as well as dispatching any message ACK or NACK requests these callbacks produce.The PR also makes sure that any received messages sitting in internal queues - but not yet dispatched to the callbacks - are automatically NACK-ed on shutdown to speed up their re-delivery (the backend is not left waiting for their ACK deadlines to expire).
PR checklist: