-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(pubsub): fix messages delivered multiple times despite a long ACK deadline #9525
Conversation
This is to prevent the messages that are put on hold from unnecessarily timing out too soon, causing the backend to re-send them.
Even the messages received that exceed the maximum load (as defined by flow control) must be lease-mananged to avoid unnecessary ACK deadline expirations, but since they are not dispatched (yet) to user callbacks, they should not contribute to the overall load. Without this change, the total load could be overestimated, resulting in an indefinitely paused message stream, and messages not being dispatched to callbacks when they should be.
This should not happen, but if it does, it is a bug in the StreamingPullManager logic, and we should know about it.
With all the messages lease-managed (even those on hold), there is no need to have a fixed default value.
@pellard1016 This pull request has not been merged yet, thus the fix is not yet available. Or did you actually try with the changes submitted here? |
Thank you for your reply Peter. I have not tried it using the changes
submitted. I will just need to wait until the merge takes place.
…On Wed, Nov 20, 2019, 15:36 Peter Lamut ***@***.***> wrote:
@pellard1016 <https://github.com/pellard1016> This pull request has not
been merged yet, thus the fix is not yet available.
Or did you actually try with the changes submitted here?
(in which case the PR needs another look)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9525?email_source=notifications&email_token=AKDMCKJK22IIF2C3B3APBNDQUWNWJA5CNFSM4JECX5MKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEUWTWI#issuecomment-556362201>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKDMCKORZTQGJ5J2K6NCIBTQUWNWJANCNFSM4JECX5MA>
.
|
pubsub/google/cloud/pubsub_v1/subscriber/_protocol/streaming_pull_manager.py
Show resolved
Hide resolved
pubsub/google/cloud/pubsub_v1/subscriber/_protocol/streaming_pull_manager.py
Show resolved
Hide resolved
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.
First pass of comments added.
I am facing the same issue, is this fix available now? |
@jawlitkp The fix for this was released with PubSub v1.1.0. In which PubSub version do you face the issue, in the latest one or in one of the older ones? |
Fixes #9252.
This PR addresses the problem with some of the messages being re-delivered too early, because their acknowledge deadline is set to short, and is not properly extended.
The issue was sometimes happening to the received messages that exceeded the flow control limits, and were thus put on hold.
The fix adds all messages to the lease management, but at the same time adjusts the load calculation to not include the messages on hold. The core pause/resume logic effectively remains the same, except that now, the ACK deadlines are automatically being extended for all received messages, and a fixed stream ACK deadline is not needed anymore.
Contrary to the original fix, this PR does not require additional client permissions.
How to test
max_messages
== 1, and make sure that the sleep interval in the message callback is longer than the current default stream ACK deadline (60 seconds).2 * sleep_delay
or so until both messages are processed and acknowledged.Actual result (before the fix):
The second message is re-delivered, because its ACK deadline was missed.
Expected result (after the fix):
Both messages are delivered only once, because their ACK deadlines were properly extended in the background.