wip: Allow retry and timeout settings on the publisher client #239
Conversation
google-cla
bot
added
the
cla: yes
cguardia
added
the
do not merge
| @@ -26,6 +26,7 @@ | |||
| from google.api_core import exceptions # type: ignore | |||
| from google.api_core import gapic_v1 # type: ignore | |||
| from google.api_core import retry as retries # type: ignore | |||
| from google.api_core import timeout as timeouts # type: ignore | |||
There was a problem hiding this comment.
This is a generated file, but I included it in this PR so that it can be seen what the synth changes will do to it.
product-auto-label
bot
added
the
api: pubsub
|
@anguillanneuf coming back to this after a few days absence. I'm not sure if this is the best way to do this. I added the publisher options and updated the docs and client, but to change the actual method definitions, I had to modify the synth code. What is the recommended way to make this kind of change? |
| topic, | ||
| data, | ||
| ordering_key="", | ||
| retry=gapic_v1.method.DEFAULT, |
There was a problem hiding this comment.
If retry and timeout are added to PublisherOptions, then there's no need to add these parameters here, yes? The user passes them in using the **attrs, like here:
There was a problem hiding this comment.
Somebody might want to override these default options for a few particular publish() calls?
I'd say we keep these two arguments for flexibility, as well as for consistency with other Publisher methods. Besides, the publish() method signature already includes the retry parameter and thus shouldn't be removed, and it makes more sense to retain timeout with it.
|
The approach of adding timeout to the PublisherOptions looks good to me. You'll have to plumb the timeout down through the sequencers to where the RPC is made. |
|
What is the status of this PR? It feels like it's lingering but I'm not sure if it's still a WIP or if it's ready to go. Thanks! |
|
@meredithslota I queried for its status. If nothing else, I can probably take over, as I expect to find some free capacity in the near future. Update: Discussed with Carlos, I'll take this over and finish whatever still needs to be done (if anything). |
|
Closing in favor of #299. |
This is a feature requested on issue #222. There are a couple of issues to discuss regarding the implementation.