Skip to content
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

Jump to bottom

fix: ACK deadline set for received messages can be too low #416

Merged
merged 6 commits into from May 26, 2021
Merged

fix: ACK deadline set for received messages can be too low #416

merged 6 commits into from May 26, 2021

Conversation

plamut
Copy link
Contributor

@plamut plamut commented May 21, 2021

Fixes #413.

This PR makes sure that the ACK deadline set for the received messages is always consistent with what the leaser uses internally when extending the ACK deadlines for the leased messages.

See the issue description and a comment explaining a possible sequence of events that lead to a bug.

PR checklist

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@plamut
fix: use the same initial ACK deadline as leaser
223a986 
When a message is received, the initial MODACK value used should be
the same value currently used by leaser. This makes sure the leaser
will wake up and extend the ACK deadline before the initial one
expires.
@plamut plamut requested a review from a team as a code owner May 21, 2021 10:22
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the googleapis/python-pubsub API. label May 21, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 21, 2021
@plamut
Make max_duration_per_lease_extension high enough
dc2b233 
If flow_control.max_duration_per_lease_extension is set to too low a
value, it is adjusted to the minimum ACK deadline.
@plamut
Copy link
Contributor Author

plamut commented May 21, 2021 •
edited

Hmm... this PR fixes the issue caused by directly using the p99 value that can be different from manager.ack_deadline used by the leaser, but even if using manager.ack_deadline, a message can still expire.

Consider - manager.ack_deadline is currently 600, because processing on of the messages took 1000 = minutes. The leaser goes to sleep for 0.9 * 600 = 540 seconds. While the leaser is asleep, more messages are received and processed quickly, dropping the histogram's p99 value to 10 seconds, consequently adjusting the value of manager._ack_deadline, too.

When yet more messages arrive, the shortened ACK deadline will be used for them while the leaser is still asleep (due to the previous "long" deadline). The leaser will not wake in time to extend the deadlines of these new messages.

Bottom line - the ACK deadline should probably not change while the leaser is asleep so that _on_response() does not use a deadline that could be shorter than the current sleep period of the leaser.

@plamut plamut added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 21, 2021
@plamut
Only update ACK deadline through leaser
ee29da4 
The ACK deadline is dynamically being adjusted based on ACK data,
but an updated deadline value must not be used for the new messages
that arrive while the leaser thread is still sleeping.

Doing so could result in the messages' ACK deadlines expiring before
the leaser wakes up and has a chance of extending the deadlines for
these messages. An updated ACK deadline value for new messages is thus
only used when the leaser starts using it, too.
@plamut
Copy link
Contributor Author

plamut commented May 21, 2021

Fixed, only the leaser now updates the manager.ack_deadline value. This means that the leased messages will never use an ACK deadline that is shorter than the duration of the leaser's current sleep period.

@plamut plamut removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 21, 2021
@plamut plamut requested a review from pradn May 21, 2021 16:30
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 21, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 21, 2021
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 22, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 22, 2021
@plamut plamut added the automerge Merge the pull request once unit tests and other checks pass. label May 26, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit e907f6e into googleapis:master May 26, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label May 26, 2021
@release-please release-please bot mentioned this pull request Aug 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimfulton jimfulton jimfulton approved these changes

@pradn pradn Awaiting requested review from pradn

Assignees
No one assigned
Labels
api: pubsub Issues related to the googleapis/python-pubsub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The initial ACK deadline set for the messages received could be too low
3 participants
@plamut @jimfulton @yoshi-kokoro