feat: retry API calls with exponential backoff #287

andrewsg · 2020-10-02T17:35:57Z

Fixes #108

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

frankyn

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

google/cloud/storage/retry.py

google/cloud/storage/hmac_key.py

google/cloud/storage/bucket.py

google/cloud/storage/retry.py

tests/unit/test_hmac_key.py

google/cloud/storage/_http.py

tests/unit/test__http.py

andrewsg · 2020-10-08T16:52:18Z

@tritone FYI

google/cloud/storage/retry.py

crwilcox · 2020-10-09T15:53:15Z

tests/unit/test__http.py

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


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?

andrewsg · 2020-10-10T00:53:38Z

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

frankyn

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

Have documentation comment requests

frankyn · 2020-10-15T05:40:42Z

google/cloud/storage/retry.py

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


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

google/cloud/storage/retry.py

andrewsg · 2020-10-15T17:17:58Z

@frankyn Added documentation as requested, PTAL

tritone

Couple small comments, generally looks really good!

tritone · 2020-10-15T17:02:57Z

google/cloud/storage/retry.py

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


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?

tritone · 2020-10-15T17:04:46Z

google/cloud/storage/retry.py

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


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

NoahDragon · 2020-11-09T21:06:16Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: retry API calls with exponential backoff #287

feat: retry API calls with exponential backoff #287

andrewsg commented Oct 2, 2020 •

edited by frankyn

frankyn left a comment

andrewsg commented Oct 8, 2020

crwilcox Oct 9, 2020

andrewsg Oct 9, 2020

andrewsg commented Oct 10, 2020

frankyn left a comment

frankyn Oct 15, 2020

tritone Oct 15, 2020

frankyn Oct 15, 2020

andrewsg Oct 15, 2020

andrewsg commented Oct 15, 2020

tritone left a comment

tritone Oct 15, 2020

tritone Oct 15, 2020

andrewsg Oct 15, 2020

NoahDragon commented Nov 9, 2020 •

edited

feat: retry API calls with exponential backoff #287

feat: retry API calls with exponential backoff #287

Conversation

andrewsg commented Oct 2, 2020 • edited by frankyn

frankyn left a comment

Choose a reason for hiding this comment

andrewsg commented Oct 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsg commented Oct 10, 2020

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsg commented Oct 15, 2020

tritone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahDragon commented Nov 9, 2020 • edited

andrewsg commented Oct 2, 2020 •

edited by frankyn

NoahDragon commented Nov 9, 2020 •

edited