Uh oh!
There was an error while loading. Please reload this page.
feat: Enable custom predicates for media operations - #1385
Conversation
cojenco
left a comment
There was a problem hiding this comment.
Looking good, very excited about retry unification!
Uh oh!
There was an error while loading. Please reload this page.
| requests.ConnectionError, | ||
| requests_exceptions.ChunkedEncodingError, | ||
| requests_exceptions.Timeout, | ||
| http.client.BadStatusLine, |
There was a problem hiding this comment.
LGTM, thanks for consolidating the retryable errors. Looking at the requests docs requests.ConnectionError should be equivalent to requests.exceptions.ConnectionError so we're good!
Uh oh!
There was an error while loading. Please reload this page.
| ) | ||
| return _request_helpers.wait_and_retry(retriable_request, self._retry_strategy) | ||
There was a problem hiding this comment.
nit: to match download, we probably want to add the retry arg docstrings for MultipartUpload, ResumableUpload etc
Uh oh!
There was an error while loading. Please reload this page.
| and the object will configure backoff and timeout options. Custom | ||
| predicates (customizable error codes) are not supported for media | ||
| operations such as this one. | ||
| and the object will configure backoff and timeout options. |
There was a problem hiding this comment.
nit: Do we want to call out the default retry? I recall we received some feedback around stating the default value in our docstrings
There was a problem hiding this comment.
I would like to incorporate DEFAULT_RETRY into the docs somehow without copy/pasting it into every method. Not sure how to do that yet. I'd prefer not to include it in this change as it's already quite large but it's a good idea.
There was a problem hiding this comment.
Sounds good, let's update that in a separate PR
This refactor unifies the old resumable media retry code with the newer google.api_core retry code, enabling custom predicates for media operations.
Fixes#1361