-
Notifications
You must be signed in to change notification settings - Fork 206
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
sample: add publish flow control sample and other nits #429
Conversation
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
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.
Looks good, except for the exception type that we want to catch.
ffe3808
to
7dd3572
Compare
7dd3572
to
6070288
Compare
except: # noqa | ||
print("Please handle {} for {}.".format(f.exception(), data)) | ||
# Wait 100 ms for the publish call to succeed. | ||
print(publish_future.result(timeout=0.1)) |
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.
Waiting only 100ms is likely to result in deadline exceeded failures. It's best not to set a deadline or if necessary, set a deadline closer to 60s.
message_id = publish_future.result() | ||
print(message_id) | ||
|
||
# Publish 1000 messages in quick succession to trigger flow control. |
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.
I wouldn't say "to trigger flow control." It's possible that flow control will be triggered, but it's also entirely possible it won't be if messages get out quickly enough. Either way, the fact that flow control was triggered isn't something that will be easily visible to the user. Maybe say "Rapidly publishing 1000 messages in a loop may be constrained by flow control."
For internal b/191088429