feat: Enable custom predicates for media operations - #1385
Conversation
Sorry, something went wrong.
cojenco
left a comment
There was a problem hiding this comment.
Looking good, very excited about retry unification!
Sorry, something went wrong.
| 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!
Sorry, something went wrong.
| ) | ||
| 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
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks, done
Sorry, something went wrong.
| 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
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Sounds good, let's update that in a separate PR
Sorry, something went wrong.
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