-
Notifications
You must be signed in to change notification settings - Fork 152
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
feat: media operation retries can be configured using the same interface as with non-media operation #447
Conversation
Tests and coverage are complete here, but docstrings are still WIP. It should be ready to be reviewed except for the docstrings. I'll mark as DNS to ensure it's not submitted without documentation complete. |
raise ValueError("num_retries and retry arguments are mutually exclusive") | ||
|
||
elif retry is not None: | ||
return resumable_media.RetryStrategy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like error types are not translatable in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean custom predicates? Yes, those are not translatable. It'll be documented wherever possible.
@@ -958,6 +970,7 @@ def _do_download( | |||
end=end, | |||
checksum=checksum, | |||
) | |||
download._retry_strategy = retry_strategy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, this is accessing a private variable maiintained in ResumableMedia, should resumable media expose a mutable setting instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I noticed that. This is the private variable we were already manipulating in our code (that was how num_retries was implemented). I don't know the history behind that. In the interest of keeping this PR small I chose not to refactor that. If we are confident about our interface we could potentially expose this in the Download and Upload constructors instead.
""" | ||
|
||
retry_strategy = _api_core_retry_to_resumable_media_retry(retry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be reasonable to expose a configurable option for mutating which errors are retried in ResumableMedia package or a pass in a lambda?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did think about that, but because both error codes and exceptions are covered, they're covered in slightly different ways and exceptions are mutated through the libraries we're using in the transport layer, I felt it was not practical for this PR. We would need to think about a more comprehensive unification of code or else an abstraction of the exceptions and error codes for that to work.
def _api_core_retry_to_resumable_media_retry(retry, num_retries=None): | ||
"""Convert google.api.core.Retry to google.resumable_media.RetryStrategy. | ||
|
||
Custom predicates are not translated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this clear to the end user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have this warning in every single public method docstring, so with that it should be clear.
|
||
:type retry: google.api_core.retry.Retry | ||
:param retry: (Optional) How to retry the RPC. A None value will disable | ||
retries. A google.api_core.retry.Retry value will enable retries, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change the default behavior to be no retries? I would expect the default value for this retry strategy to be some reasonable standard policy rather than None. Or is this okay here because the method is not one that end users will call directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That last bit is correct; private methods such as this one don't have a retry default, but public methods do and they pass them through.
# arguments into query_params dictionaries. Media operations work | ||
# differently, so here we make a "fake" query_params to feed to the | ||
# ConditionalRetryPolicy. | ||
query_params = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If if_generation_match
and if_metageneration_match
are not set, will the values in this dict be 0 or None
? I'm a little confused because the docstring indicates that the param is optional but the type is listed aslong
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They will be None. Optional in the docstring means it may be either the declared type or None.
num_retries = 0 | ||
|
||
# Handle ConditionalRetryPolicy. | ||
if isinstance(retry, ConditionalRetryPolicy): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will there be cases where if_metageneration_match is not None
and retry=None
? Or is it not a concern to this private method? IIUC, it seems like if_metageneration_match
will be picked up as query params in the upload url eventually, but at the same time we will have retry as resumable_media.RetryStrategy(max_retries=0)
? Will the conditional retry still be triggered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if_metageneration_match
not being None
and retry
being None
is a valid state here. query_params
inside this conditional only exists for the purpose of evaluating a ConditionalRetryPolicy
. In any scenario where there is no ConditionalRetryPolicy
, this code will not fire, but if_generation_match
and if_metageneration_match
will still be consumed properly and added to the request later in the process.
No region tags are edited in this PR.This comment is generated by snippet-bot.
|
3caedef
to
4b37672
Compare
Currently working through some absolutely brutal test merge conflicts, will ask for further review when done. |
@tseaver PTAL. Thank you for your attention, I have much more confidence in the merge now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Andrew!
…ace as with non-media operation (googleapis#447) All media operation calls (downloads and uploads) can be configured with Retry objects and ConditionalRetryPolicy objects, nearly identically to non-media operations. This is accomplished by converting the Retry object to a google-resumable-media-python library RetryStrategy object at the point of entry to that library. Custom predicates of Retry objects (for instance set with Retry(predicate=...)) are not supported for media operations; they will be replaced with a media-operation-specific predicate. This change is backwards-compatible for users of public methods using num_retries arguments to configure uploads; num_retries continue to be supported but the deprecation warning remains in effect. They will be fully removed and replaced with Retry objects in the future. With this change, the default parameters for a media operations retry changes to be uniform with non-media operation retries. Specifically, the retry deadline for media operation retries becomes 120 seconds unless otherwise configured.
…ace as with non-media operation (googleapis#447) All media operation calls (downloads and uploads) can be configured with Retry objects and ConditionalRetryPolicy objects, nearly identically to non-media operations. This is accomplished by converting the Retry object to a google-resumable-media-python library RetryStrategy object at the point of entry to that library. Custom predicates of Retry objects (for instance set with Retry(predicate=...)) are not supported for media operations; they will be replaced with a media-operation-specific predicate. This change is backwards-compatible for users of public methods using num_retries arguments to configure uploads; num_retries continue to be supported but the deprecation warning remains in effect. They will be fully removed and replaced with Retry objects in the future. With this change, the default parameters for a media operations retry changes to be uniform with non-media operation retries. Specifically, the retry deadline for media operation retries becomes 120 seconds unless otherwise configured.
All media operation calls (downloads and uploads) can be configured with Retry objects and ConditionalRetryPolicy objects, nearly identically to non-media operations. This is accomplished by converting the Retry object to a google-resumable-media-python library RetryStrategy object at the point of entry to that library.
Custom predicates of Retry objects (for instance set with Retry(predicate=...)) are not supported for media operations; they will be replaced with a media-operation-specific predicate.
This change is backwards-compatible for users of public methods using num_retries arguments to configure uploads; num_retries continue to be supported but the deprecation warning remains in effect. They will be fully removed and replaced with Retry objects in the future.
With this change, the default parameters for a media operations retry changes to be uniform with non-media operation retries. Specifically, the retry deadline for media operation retries becomes 120 seconds unless otherwise configured.