pubsub: use unary calls to send acks/modacks #3123
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
Previously we send ack/modack requests on the StreamingPull streams. However, if pull responses are buffered, gRPC cannot promptly tell us that the stream is broken, so we'll keep sending requests on the dead stream. This commit implements a temporary solution. We send requests by unary RPCs instead and only use streaming for receiving messages. Load test shows no regression. However, modacks seem to take longer to take effect. The timeout for nack test had to be increased.
googlebot
added
the
cla: yes
|
This LGTM. It is expected this would add additional latency to ack/mod ack operations, so all buffer amounts should be increased accordingly, but this looks good. |
|
For the record, this fix may also have the side effect of properly ack-ing messages when the subscriber is shutdown with acks outstanding. I was tracking a problem where acks were getting lost and it goes away in 0.43 which includes this PR. Thanks! |
|
This also might actually be the change that fixed #3065. |
Contributor
Author
|
Thank you for tracking this down. This seems to be a better change than expected... 😆 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Previously we send ack/modack requests on the StreamingPull streams.
However, if pull responses are buffered, gRPC cannot promptly tell us
that the stream is broken, so we'll keep sending requests on the dead
stream.
This commit implements a temporary solution. We send requests by unary
RPCs instead and only use streaming for receiving messages.
Load test shows no regression. However, modacks seem to take longer to
take effect. The timeout for nack test had to be increased.
cc @mdietz94