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

process: set publisher methods' max retry to 600s #495

Merged
merged 3 commits into from Sep 24, 2021

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Sep 3, 2021

Fixes #494.

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 review from a team as code owners September 3, 2021 13:56
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 3, 2021
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the googleapis/python-pubsub API. label Sep 3, 2021
@plamut plamut added owlbot:run Add this label to trigger the Owlbot post processor. and removed api: pubsub Issues related to the googleapis/python-pubsub API. cla: yes This human has signed the Contributor License Agreement. labels Sep 3, 2021
@google-cla
Copy link

google-cla bot commented Sep 3, 2021

☹️ Sorry, but only Googlers may change the label cla: yes.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 3, 2021
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 3, 2021
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

This change, while expedient, is a bit ugly: the deadlines are emitted from this template, based on this method configuration in the googleapis repository.

If there is a general usecase for setting the deadlines to a higher multiple of the proto-repository-specified timeout, then we should have an issue open against gapic-generator-python to support it (and maybe we need additional configuration available in the service JSON config).

@product-auto-label product-auto-label bot added the api: pubsub Issues related to the googleapis/python-pubsub API. label Sep 4, 2021
@plamut
Copy link
Contributor Author

plamut commented Sep 5, 2021

Yeah, the thing is that currently the same value is used for retry deadline and the default timeout, thus indeed a generator change would be required to support it.

I can check at the next Python libs weekly if this is something other libraries could use as well, or whether it's just something specific to Pub/Sub clients - @kamalaboulhosn do you maybe know from the top of your head?

@plamut
Copy link
Contributor Author

plamut commented Sep 7, 2021

Update: As discussed offline, we will go with this patchy version until it is decided where (and if) in the generator should this be changed.

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

Yeah, this does look a little hacky. I guess this is something that comes from the service config. @hongalex is this a config we can change in a single place that can be distributed to the repos so we don't to do these ad-hoc changes?

@plamut
Copy link
Contributor Author

plamut commented Sep 8, 2021

The problem here is that the gapic code generator currently uses the same setting for both the timeout and the retry deadline (method.timeout setting from here).

FWIW, it was agreed on the Python libs weekly yesterday that it needs to be investigated whether this is a quirk in the generator, or a lack of feature that would have to be implemented. Until then, the OwlBot replacement rule should be fine in order to not block this P1 change for too long.

@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 Sep 24, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 24, 2021
@plamut plamut merged commit 7e623e5 into googleapis:main Sep 24, 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 Sep 24, 2021
@plamut plamut deleted the iss-494 branch September 24, 2021 06:50
@plamut plamut mentioned this pull request Sep 24, 2021
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.

Set default publisher retry and deadline settings to be consistent with desired values
4 participants