feat: retry API calls with exponential backoff by andrewsg · Pull Request #287 · googleapis/python-storage · GitHub

Fixes #108

Related: https://github.com/googleapis/google-cloud-python/issues/9298

Thank you for your patience, it took a bit more time than I expected to review the PR.

this test is missing the comment the other tests have. There may be a way to refactor this also, but unsure if it is warranted.

Which comment is missing?

@crwilcox @frankyn PTAL. Changes include:

Retry predicate simplified; my testing suggests the google-api-core nuanced inspection of error messages is not needed for GCS.
Retry predicate now includes all HTTP error codes listed in retry design, except 408
-- 408 is mysteriously missing from google-api-core exceptions and I have no way to test what happens if we actually get one (testbench was no help here)
Retry predicate now includes ConnectionError which encompasses failure to find host and broken connections of all types. LMK if we need to narrow it down; we have fairly rich and detailed info on the specific ConnectionError subtype which we can use here.

Responded to all other comments as well.

Generally LGTM, clean design with minimal impact on the code base!

Have documentation comment requests

@tritone this is a change in what we discussed around retrying ConnectionErrors in general. Discussed this with @andrewsg w.r.t retrying unable to connect errors and for now implementation is moving forward with this solution until it's baked into the api_core libraries. Rationale, there's a default timeout enabled that would timeout the retry compared to Golang which does not.

Got it, this seems fine to me re: the fact that there is a default timeout in python. So the idea is that down the road, we can add an error in api_core which catches "connection reset by peer" more specifically?

that's correct, because right now requests.ConnectionError is tied to a specific implementation, e.g. requests and should be wrapped by api_core / cloud_core for it to be used generally outside of just the GCS library.

I'm not totally convinced that retrying for a little while by default if there is an interruption due to network outage is not the right thing to do anyways, but yes, the idea is that instead of tapping into requests here we would have a custom api_core exception like we do with other errors.

@frankyn Added documentation as requested, PTAL

Couple small comments, generally looks really good!

Got it, this seems fine to me re: the fact that there is a default timeout in python. So the idea is that down the road, we can add an error in api_core which catches "connection reset by peer" more specifically?

Would it be worth having a short sample snippet here rather than explaining and inlining? Might make it easier to understand what's necessary.

I decided against this since a useful snippet here would have to show how to actually inject the modified retry via user code, and that feature is out of scope for this PR (will require changing the signature of every method in the library).

Just wonder if the blob.update() also has the retry logic with this feature. Is there any way to specify the retry number?

Update: followed the API core doc, using DEFAULT_RETRY as decorator seems works. But how can I confirm if the retry happened?

Update: there is a fix on this PR #340

Retries errors for idempotent API calls by default. Some API calls are conditionally idempotent (only idempotent if etag, generation, if_generation_match, if_metageneration_match are specified); in those cases, retries are also conditional on the inclusion of that data in the call.

andrewsg added 5 commits October 2, 2020 09:44

unit tests pass

8d3ff6a

_http tests

0c48398

blacken

054710d

lint

186362f

notes

bbf10d7

andrewsg requested a review from frankyn October 2, 2020 17:35

google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 2, 2020

frankyn added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 2, 2020

frankyn requested a review from crwilcox October 2, 2020 18:33

frankyn suggested changes Oct 7, 2020

View reviewed changes

crwilcox reviewed Oct 9, 2020

View reviewed changes

andrewsg added 5 commits October 9, 2020 11:15

change etag behavior

741aff0

Merge branch 'master' into retry

403afd4

py2 compat

95293c1

comments

73d37cf

Support expanded error types for retry; retry ConnectionError; lint

46693d7

andrewsg added 3 commits October 12, 2020 09:24

py2 compat

7052b12

add support for 408 exceptions

5f0f473

lint

8ba68c4

frankyn suggested changes Oct 15, 2020

View reviewed changes

andrewsg removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 15, 2020

documentation

ffa6527

tritone reviewed Oct 15, 2020

View reviewed changes

andrewsg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 15, 2020

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

Merge branch 'master' into retry

1d46d1c

frankyn approved these changes Oct 15, 2020

View reviewed changes

andrewsg merged commit fbe5d9c into master Oct 16, 2020

andrewsg deleted the retry branch October 16, 2020 19:21

release-please Bot mentioned this pull request Jan 12, 2022

chore(main): release 3.0.0 #688

Closed

+                      response.status_code = 200
+                      data = b"brent-spiner"
+                      response._content = data
+                      http.request.return_value = response

+                  exceptions.BadGateway,  # 502
+                  exceptions.ServiceUnavailable,  # 503
+                  exceptions.GatewayTimeout,  # 504
+                  requests.ConnectionError,

+                  exceptions.BadGateway,  # 502
+                  exceptions.ServiceUnavailable,  # 503
+                  exceptions.GatewayTimeout,  # 504
+                  requests.ConnectionError,

+              To modify the default retry behavior, call a ``with_XXX`` method
+              on ``DEFAULT_RETRY``. For example, to change the deadline to 30 seconds,
+              pass ``retry=DEFAULT_RETRY.with_deadline(30)``. See google-api-core reference
+              (https://googleapis.dev/python/google-api-core/latest/retry.html) for details.

feat: retry API calls with exponential backoff by andrewsg · Pull Request #287 · googleapis/python-storage · GitHub

Uh oh!

feat: retry API calls with exponential backoff - #287

feat: retry API calls with exponential backoff#287andrewsg merged 15 commits intomastergoogleapis/python-storage:masterfrom retrygoogleapis/python-storage:retryCopy head branch name to clipboard

Conversation

andrewsg commented Oct 2, 2020 • edited by frankyn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewsg commented Oct 8, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewsg commented Oct 10, 2020

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewsg commented Oct 15, 2020

Uh oh!

tritone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoahDragon commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

feat: retry API calls with exponential backoff#287
andrewsg merged 15 commits into
masterfrom
retry

andrewsg commented Oct 2, 2020 •
edited by frankyn
Loading

NoahDragon commented Nov 9, 2020 •
edited
Loading