feat: exactly-once delivery support by pradn · Pull Request #550 · googleapis/python-pubsub

pradn · 2021-12-13T22:12:21Z

Add new field to SubscriptionProperties and modify the GAPIC layer with just this one change. This change will come from the auto-GAPIC generated code once the proto change is public.
Add async ack/modack/nack methods to the Message class. Exactly-once users have to use this to get the status of ack/modack/nack requests. The futures returned by these methods may be completed by the AcknowledgeError exception (with detailed AcknowledgeStatus status code) or a success AcknowledgeStatus code. These futures are of a new type, one used only on the subscriber-side for propagating exactly-once errors. This future mirrors the publishing-side future that already exists. The futures are completed immediately if exactly-once delivery is not enabled, upon successful ack/modack completion, and when errors/exceptions occur.
Split unary ack/modack methods into two in the subscriber manager. Some repeated code but it's simpler this way.
Keep track of whether exactly-once is enabled for a subscription by looking at the subscription's SubscriptionProperties, returned in the streaming response.
Set the minimum ack deadline to 60 secs if exactly-once is known to be turned on. This deadline is used by the leaser.
Add new min-lease-extension parameter. If the user sets this, it overrides the auto-set param based on whether exactly-once is enabled or not.
Retry unary ack/modacks if we get errors in the GRPC call's ErrorInfo (no retry delays as of yet since the server will eventually fail them if the ack expires). This is done in the dispatcher, which retries only the acks/modacks that have failed temporarily. Permanent failures are surfaced to the user as future exceptions. These retries use an exponential backoff with a max of 10 min, which is sufficient since the server allows ack/modacks for 10 mins max. These retries are triggered on a new thread to prevent the dispatcher thread from being blocked by retry-wait sleeps.
Retry lease modacks, both immediately upon receipt of messages and also in the background. If retries fail or if permanent errors arise, we log the errors but don't otherwise disrupt user code.
Send a new ack deadline via stream_ack_deadline_seconds whenever the exactly_once setting for a stream changes.
Add tests ensuring 100% coverage
Usage sample for this feature added at samples: sample for receiving messages with exactly-once delivery enabled #588

plamut

This is only a partial feedback, I have not gone through the entire PR yet.

The think we need to be careful about is to not block any worker threads for too long, e.g. when retrying failed requests.

plamut

Round two, added some additional comments.

Will review the streaming pull manager tests later.

plamut

Went through the rest of the PR, this should be it for the first review round. :)

plamut · 2022-01-13T13:05:27Z

@pradn I see a lot of comments marked as resolved, but no changes in the code - is there a git push missing? :)

pradn

Pushed

snippet-bot · 2022-02-16T22:10:05Z

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

acocuzzo

After conversation: https://github.com/googleapis/python-pubsub/pull/550/files#r769774809
is resolved, and all checks pass, LGTM.

…y-once delivery is not enabled.

…esn't break.

…ingPull stream is being shutdown.

…futures

pradn requested a review from plamut December 13, 2021 22:12

product-auto-label bot added the api: pubsub Issues related to the googleapis/python-pubsub API. label Dec 13, 2021

pradn changed the title ~~Exactly-once changes (draft)~~ feat: exactly-once changes (draft) Dec 13, 2021

plamut reviewed Dec 15, 2021

View reviewed changes

plamut suggested changes Dec 16, 2021

View reviewed changes

plamut reviewed Dec 16, 2021

View reviewed changes

plamut reviewed Dec 22, 2021

View reviewed changes

Comment thread google/cloud/pubsub_v1/subscriber/_protocol/streaming_pull_manager.py Outdated

pradn commented Jan 19, 2022

View reviewed changes

maheshgattani reviewed Feb 8, 2022

View reviewed changes

pradn commented Feb 8, 2022

View reviewed changes

Comment thread google/cloud/pubsub_v1/subscriber/_protocol/streaming_pull_manager.py Outdated

tswast reviewed Feb 23, 2022

View reviewed changes

Comment thread tests/unit/pubsub_v1/subscriber/test_futures_subscriber.py Outdated

acocuzzo marked this pull request as ready for review February 23, 2022 21:38

acocuzzo requested a review from a team February 23, 2022 21:38

acocuzzo requested a review from a team as a code owner February 23, 2022 21:38

acocuzzo requested review from a team and nicain February 23, 2022 21:38

acocuzzo approved these changes Feb 23, 2022

View reviewed changes

anguillanneuf reviewed Feb 24, 2022

View reviewed changes

Comment thread samples/snippets/subscriber.py Outdated

anguillanneuf reviewed Feb 24, 2022

View reviewed changes

Comment thread samples/snippets/subscriber.py Outdated

maheshgattani reviewed Feb 24, 2022

View reviewed changes

maheshgattani reviewed Feb 25, 2022

View reviewed changes

Comment thread google/cloud/pubsub_v1/subscriber/_protocol/dispatcher.py

pradn force-pushed the exactly-once2 branch from 32fbbb4 to 2453ef4 Compare February 25, 2022 22:26

maheshgattani approved these changes Feb 25, 2022

View reviewed changes

pradn removed request for a team and nicain February 25, 2022 22:45

pradn added 3 commits March 1, 2022 14:09

Exactly-once changes

ed5d6e5

rename retry duration constants

e35de42

add grpc-status dependency

23504c8

pradn added 21 commits March 1, 2022 14:09

Reformat files

57235b9

More fixes

10f5a93

Improve coverage

cdb6340

Improve coverage

03c8b54

lint

8099fde

Retry on a new thread to avoid blocking the one dispatcher thread.

7edee1d

Remove sample - will be pulled into separate PR.

6fe7bf8

Fix type checking errors.

1d209c9

Address some of Mahesh's comments

3facfa5

Return AcknowledgeStatus.SUCCESS for _with_response methods if exactl…

c186713

…y-once delivery is not enabled.

Get coverage to 100%

8fb9d56

Add default value to new Message ctor parameter so PubSubLite code do…

898715b

…esn't break.

Complete futures when a permanent RetryError is thrown and the Stream…

245cdfd

…ingPull stream is being shutdown.

Fix lint

b1e5d70

Fix coverage false positive in test by ignoring it.

377222b

Reword some comments with "exactly-once delivery"

a20d48c

Remove debug print

3b390b7

Fix coverage false positive in test by ignoring it.

9fa28c5

Complete all requests, not just ones with futures

c4ba9c7

Better names to reflect them applying to all reqs not just ones with …

36fcab9

…futures

Improve tests and comments

4158315

pradn force-pushed the exactly-once2 branch from 372badb to 4158315 Compare March 1, 2022 19:09

pradn changed the title ~~feat: exactly-once changes (draft)~~ feat: exactly-once changes Mar 1, 2022

pradn changed the title ~~feat: exactly-once changes~~ feat: exactly-once delivery support Mar 1, 2022

Merge branch 'main' into exactly-once2

eddd267

pradn merged commit 2fb6e15 into googleapis:main Mar 4, 2022

This was referenced Mar 4, 2022

chore(main): release 2.10.0 #567

Closed

chore(main): release 2.10.0 #603

Merged

simonvanderveldt mentioned this pull request May 4, 2022

Assert fails with duplicate AckRequests #661

Closed

release-please bot mentioned this pull request Aug 11, 2022

chore(main): release 2.13.6 #761

Merged

Conversation

pradn commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plamut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plamut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plamut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plamut commented Jan 13, 2022

Uh oh!

pradn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snippet-bot bot commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No region tags are edited in this PR.

Uh oh!

Uh oh!

acocuzzo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pradn commented Dec 13, 2021 •

edited

Loading

snippet-bot bot commented Feb 16, 2022 •

edited

Loading