feat: support customizable retry and timeout settings on the publisher client #299

plamut · 2021-02-26T22:25:55Z

Closes #222.
Supersedes #239.

This PR adds support for API core timeout classes to the publisher class, replacing the previous float timeouts.

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)

google-cla · 2021-02-26T22:26:01Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

cguardia · 2021-02-27T02:28:16Z

@googlebot I consent.

jimfulton

I think the type UNION should admit floats and ints.

I also think the union should be defined once and reused.

There's an obscene amount of repetition in this package. It was already there, but may be can not add to it. :)

UPGRADING.md

google/cloud/pubsub_v1/types.py

google/cloud/pubsub_v1/publisher/client.py

jimfulton · 2021-05-28T18:51:11Z

google/cloud/pubsub_v1/types.py

@@ -37,6 +39,16 @@
 from google.pubsub_v1.types import pubsub as pubsub_gapic_types


+TimeoutType = Union[
+    None,


Did we mean to add None? Was this my fault?

This feels like an unintentional change. We didn't accept None before.

If we want to accept None, then I think we should update

https://github.com/googleapis/python-api-core/blob/7337c6b123735fb9ae5d0e54b4399275719c020c/google/api_core/gapic_v1/method.py#L66-L67

to treat None like DEFAULT.

Actually we did, even though the argument description says otherwise. But the generated code actually used None as a default value for a supposedly float-only argument. :)

jimfulton · 2021-05-28T18:55:08Z

google/pubsub_v1/services/publisher/async_client.py

@@ -315,6 +313,9 @@ async def update_topic(
            ),
        )

+        if timeout is None or isinstance(timeout, (int, float)):
+            timeout = timeouts.ConstantTimeout(timeout)


We shouldn't need this except for the None case, which is new. I don't think we should accept None. Apologies if None came from me.

int and float are handled here:

https://github.com/googleapis/python-api-core/blob/7337c6b123735fb9ae5d0e54b4399275719c020c/google/api_core/gapic_v1/method.py#L66-L67

The same comment applies to other places where you added this.

And, of course, not DRY.

jimfulton · 2021-05-28T19:10:40Z

google/cloud/pubsub_v1/publisher/_sequencer/base.py

@@ -45,18 +47,17 @@ def unpause(self, message):  # pragma: NO COVER

    @staticmethod
    @abc.abstractmethod
-    def publish(self, message, retry=None, timeout=None):  # pragma: NO COVER
+    def publish(
+        self, message, retry=None, timeout: gapic_types.TimeoutType = None


Can we use DEFAULT rather than None? The implementation defaults to DEFAULT.

That makes sense, the ABC should be consistent.

jimfulton

Thanks for these changes.

My only complaint is that we added None as a valid timeout, which causes some pain.

plamut · 2021-05-28T19:18:11Z

@jimfulton I added None, because I noticed that ConstantTimeout actually supports it.

(not saying that explicitly disabling any timeouts is a good idea, but we already support that automaticlally if we allow instances of ConstantTimeout)

plamut · 2021-05-28T19:23:59Z

Flaky samples tests, 500 internal.

jimfulton · 2021-05-28T19:32:00Z

@jimfulton I added none, because I noticed that ConstantTimeout actually supports it.

(not saying that explicitly disabling any timeouts is a good idea, but we already support that automaticlally if we allow instances of ConstantTimeout)

That's fair, but at least you could argue that we should make people work to disable timeouts.

BTW, I tried to figure out what the effect of passing None to ConstantTimout would be and got lost in the web of indirection. :) To convince myself that I knew what was going on I'd have to spend some quality time with pdb, and I don't have time for that today.

In any case, if we want to accept None, I think we should handle that in one place (https://github.com/googleapis/python-api-core/blob/7337c6b123735fb9ae5d0e54b4399275719c020c/google/api_core/gapic_v1/method.py#L66-L67) so we don't repeat the logic in many places.

plamut · 2021-05-28T19:37:21Z

I actually don't see a good reason why somebody wanted to potentially block indefinitely, thus I'm fine with removing support for a plain None (one can always pass in ConstantTimeout(None), if they really want to). We can then also get rid of the "convert constant to object" code in the generated code, because number constants are already handled in api-core.

Using no timeout is not a good idea, but if one really wants to, they can pass it in as ConstantTimeout(None). As a side effect, the logic of converting a constant into a COnstantTimeout instance can be removed, as this is already handled in api-core for int and float values.

plamut · 2021-05-28T19:59:06Z

BTW, I tried to figure out what the effect of passing None to ConstantTimout would be and got lost in the web of indirection. :)

If I read it correctly, a ConstantTimeout(None) is used as an RPC method decorator. What ConstantTimeout does is just setting the timeout argument to the wrapped method to the given value (None in this case).

plamut · 2021-06-15T17:32:26Z

Note to self - this PR modifies synth.py, thus after merging, we also need to update #411

cc: @parthea

* chore: migrate to owl bot * chore: copy files from googleapis-gen 40278112d2922ec917140dcb5cc6d5ef2923aeb2 * chore: run the post processor * pull in synth.py changes from #299 * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * .github/use gcr.io/cloud-devrel-public-resources/owlbot-python post processor image * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * fix owlbot.py replacement * Copy generated code from googleapis-gen * Work around gapic generator docstring bug * fix: require google-api-core >= 1.26.0 * Work around gapic generator docstring bug * fix(deps): add packaging requirement * revert .coveragerc Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

cguardia and others added 10 commits November 16, 2020 01:02

feat: allow retry and timeout settings on publisher client

729a5e7

build: update generated code and update doc

a9b5190

Merge branch 'master' into retry-timeout-settings

1b8767b

Merge branch 'master' into retry-timeout-settings

b06d122

Merge branch 'master' into iss-222

54a1fd6

Propagate publish timeout to the RPC call

12f2140

test publisher client

240f992

Fix timeout parameter type in docstrings

272ccdc

Apply timeout changes to async publisher client

a0a1fcb

Merge branch 'master' into iss-222

2d0046c

plamut requested a review from a team as a code owner February 26, 2021 22:25

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

google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Feb 26, 2021

plamut mentioned this pull request Feb 26, 2021

wip: Allow retry and timeout settings on the publisher client #239

Closed

plamut changed the title ~~Iss 222~~ feat: support customizable retry and timeout settings on the publisher client Feb 26, 2021

google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Feb 27, 2021

Merge branch 'master' into iss-222

3ad5779

plamut requested a review from pradn April 28, 2021 10:20

plamut added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Apr 30, 2021

yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Apr 30, 2021

jimfulton self-requested a review May 14, 2021 12:32

jimfulton reviewed May 16, 2021

View reviewed changes

UPGRADING.md Outdated Show resolved Hide resolved

google/cloud/pubsub_v1/types.py Outdated Show resolved Hide resolved

google/cloud/pubsub_v1/publisher/client.py Outdated Show resolved Hide resolved

plamut added 3 commits May 28, 2021 19:39

Introduce TimeoutType type alias

ce8daea

Update PublisherOptions docs

2adaba1

Use type alias for timeout in upgrading guide

072d636

jimfulton reviewed May 28, 2021

View reviewed changes

plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 28, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 28, 2021

Use DEFAULT as a defualt timeout in base sequencer

762b129

plamut requested a review from jimfulton June 8, 2021 12:55

jimfulton approved these changes Jun 15, 2021

View reviewed changes

plamut added automerge Merge the pull request once unit tests and other checks pass. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 15, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 15, 2021

plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 15, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 15, 2021

plamut mentioned this pull request Jun 15, 2021

chore: migrate to owl bot #411

Merged

plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 15, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 15, 2021

plamut merged commit 7597604 into googleapis:master Jun 15, 2021

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jun 15, 2021

plamut deleted the iss-222 branch June 15, 2021 18:02

parthea added a commit that referenced this pull request Jul 17, 2021

pull in synth.py changes from #299

c842cf0

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

chore(main): release 2.13.6 #761

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support customizable retry and timeout settings on the publisher client #299

feat: support customizable retry and timeout settings on the publisher client #299

plamut commented Feb 26, 2021 •

edited

Loading

google-cla bot commented Feb 26, 2021

cguardia commented Feb 27, 2021

jimfulton left a comment •

edited

Loading

jimfulton May 28, 2021 •

edited

Loading

plamut May 28, 2021 •

edited

Loading

jimfulton May 28, 2021 •

edited

Loading

jimfulton May 28, 2021

jimfulton May 28, 2021

plamut May 28, 2021

jimfulton left a comment

plamut commented May 28, 2021 •

edited

Loading

plamut commented May 28, 2021

jimfulton commented May 28, 2021

plamut commented May 28, 2021 •

edited

Loading

plamut commented May 28, 2021 •

edited

Loading

plamut commented Jun 15, 2021

feat: support customizable retry and timeout settings on the publisher client #299

feat: support customizable retry and timeout settings on the publisher client #299

Conversation

plamut commented Feb 26, 2021 • edited Loading

google-cla bot commented Feb 26, 2021

cguardia commented Feb 27, 2021

jimfulton left a comment • edited Loading

Choose a reason for hiding this comment

jimfulton May 28, 2021 • edited Loading

Choose a reason for hiding this comment

plamut May 28, 2021 • edited Loading

Choose a reason for hiding this comment

jimfulton May 28, 2021 • edited Loading

Choose a reason for hiding this comment

jimfulton May 28, 2021

Choose a reason for hiding this comment

jimfulton May 28, 2021

Choose a reason for hiding this comment

plamut May 28, 2021

Choose a reason for hiding this comment

jimfulton left a comment

Choose a reason for hiding this comment

plamut commented May 28, 2021 • edited Loading

plamut commented May 28, 2021

jimfulton commented May 28, 2021

plamut commented May 28, 2021 • edited Loading

plamut commented May 28, 2021 • edited Loading

plamut commented Jun 15, 2021

plamut commented Feb 26, 2021 •

edited

Loading

jimfulton left a comment •

edited

Loading

jimfulton May 28, 2021 •

edited

Loading

plamut May 28, 2021 •

edited

Loading

jimfulton May 28, 2021 •

edited

Loading

plamut commented May 28, 2021 •

edited

Loading

plamut commented May 28, 2021 •

edited

Loading

plamut commented May 28, 2021 •

edited

Loading