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: support customizable retry and timeout settings on the publisher client #299

Merged
merged 19 commits into from Jun 15, 2021

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Feb 26, 2021

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)

@plamut plamut requested a review from a team as a code owner February 26, 2021 22:25
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the googleapis/python-pubsub API. label Feb 26, 2021
@google-cla
Copy link

google-cla bot commented Feb 26, 2021

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.

@google-cla google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Feb 26, 2021
@plamut plamut changed the title Iss 222 feat: support customizable retry and timeout settings on the publisher client Feb 26, 2021
@cguardia
Copy link
Contributor

@googlebot I consent.

@google-cla 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
@plamut plamut requested a review from pradn April 28, 2021 10:20
@plamut 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 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 jimfulton self-requested a review May 14, 2021 12:32
Copy link
Contributor

@jimfulton jimfulton left a comment

Choose a reason for hiding this comment

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

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 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
@@ -37,6 +39,16 @@
from google.pubsub_v1.types import pubsub as pubsub_gapic_types


TimeoutType = Union[
None,
Copy link
Contributor

@jimfulton jimfulton May 28, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@plamut plamut May 28, 2021

Choose a reason for hiding this comment

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

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. :)

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

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

@jimfulton jimfulton May 28, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

And, of course, not DRY.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, the ABC should be consistent.

Copy link
Contributor

@jimfulton jimfulton left a comment

Choose a reason for hiding this comment

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

Thanks for these changes.

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

@plamut
Copy link
Contributor Author

plamut commented May 28, 2021

@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
Copy link
Contributor Author

plamut commented May 28, 2021

Flaky samples tests, 500 internal.

@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 28, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 28, 2021
@jimfulton
Copy link
Contributor

@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
Copy link
Contributor Author

plamut commented May 28, 2021

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
Copy link
Contributor Author

plamut commented May 28, 2021

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 plamut requested a review from jimfulton June 8, 2021 12:55
@plamut 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 yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 15, 2021
@plamut
Copy link
Contributor Author

plamut commented Jun 15, 2021

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

cc: @parthea

@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 15, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 15, 2021
@plamut plamut mentioned this pull request Jun 15, 2021
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 15, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 15, 2021
@plamut plamut merged commit 7597604 into googleapis:master Jun 15, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jun 15, 2021
@plamut plamut deleted the iss-222 branch June 15, 2021 18:02
parthea added a commit that referenced this pull request Jul 17, 2021
plamut pushed a commit that referenced this pull request Jul 19, 2021
* 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>
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 googleapis/python-pubsub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow retry and timeout settings on the publisher client
4 participants