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 as a code owner Feb 26, 2021
@google-cla
Copy link

@google-cla 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.

Loading

@google-cla google-cla bot added the cla: no 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

@cguardia cguardia commented Feb 27, 2021

@googlebot I consent.

Loading

@google-cla google-cla bot added cla: yes and removed cla: no labels Feb 27, 2021
Copy link
Contributor

@jimfulton jimfulton left a comment

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

Loading

UPGRADING.md Outdated Show resolved Hide resolved
Loading
google/cloud/pubsub_v1/types.py Outdated Show resolved Hide resolved
Loading
google/cloud/pubsub_v1/publisher/client.py Outdated Show resolved Hide resolved
Loading
@plamut plamut requested a review from jimfulton May 28, 2021
@@ -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.

Loading

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

Loading

@@ -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.

Loading

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.

And, of course, not DRY.

Loading

@@ -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

@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.

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

Loading

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.

That makes sense, the ABC should be consistent.

Loading

Copy link
Contributor

@jimfulton jimfulton left a comment

Thanks for these changes.

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

Loading

@plamut
Copy link
Contributor Author

@plamut 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)

Loading

@plamut
Copy link
Contributor Author

@plamut plamut commented May 28, 2021

Flaky samples tests, 500 internal.

Loading

@jimfulton
Copy link
Contributor

@jimfulton jimfulton 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)

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.

Loading

@plamut
Copy link
Contributor Author

@plamut 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.

Loading

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

Loading

@plamut plamut requested a review from jimfulton Jun 8, 2021
@plamut
Copy link
Contributor Author

@plamut plamut commented Jun 15, 2021

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

cc: @parthea

Loading

@plamut plamut mentioned this pull request Jun 15, 2021
@plamut plamut merged commit 7597604 into googleapis:master Jun 15, 2021
9 checks passed
Loading
@plamut plamut deleted the iss-222 branch Jun 15, 2021
parthea added a commit that referenced this issue Jul 17, 2021
plamut pushed a commit that referenced this issue 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants