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

feat(pubsub): add stop method #9365

Merged
merged 15 commits into from
Nov 7, 2019
Merged

Conversation

IlyaFaer
Copy link

@IlyaFaer IlyaFaer commented Oct 1, 2019

Add stop() method, which sends all outstanding messages and waits until all futures resolved. Similar features in Go and Java.

Closes #4913
Closes #6883

@IlyaFaer IlyaFaer added the api: pubsub Issues related to the Pub/Sub API. label Oct 1, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 1, 2019
@IlyaFaer IlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 1, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 1, 2019
@IlyaFaer IlyaFaer marked this pull request as ready for review October 1, 2019 09:38
@IlyaFaer IlyaFaer requested a review from plamut as a code owner October 1, 2019 09:38
@pradn pradn self-requested a review October 2, 2019 16:31
@googleapis googleapis deleted a comment from tseaver Oct 4, 2019
Copy link
Contributor

@pradn pradn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@IlyaFaer
Copy link
Author

How about:
to make sure all publish requests completed, either in success or error.

No problem, pushed

@IlyaFaer
Copy link
Author

IlyaFaer commented Nov 7, 2019

@pradn, thanks for leading me through
@kamalaboulhosn, well, it's now more close to what you proposed. Do you have any more comments? Or we could merge, I think

Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, including the locking (the code is also easier to reason about with the _batch_lock spanning across the entire bodies of the publish/stop methods).

Suggested two small improvements, if it's not too late (had to prioritize other stuff, apologies).

Edit: To clarify, this PR can be merged as-is if it's blocking other @pradn's work, and the suggested changes, if accepted, can be added separately.

pubsub/google/cloud/pubsub_v1/publisher/client.py Outdated Show resolved Hide resolved
@kamalaboulhosn
Copy link

@pradn, thanks for leading me through
@kamalaboulhosn, well, it's now more close to what you proposed. Do you have any more comments? Or we could merge, I think

Works for me, thanks!

Co-Authored-By: Peter Lamut <plamut@users.noreply.github.com>
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the quick changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
7 participants