feat: media operation retries can be configured using the same interface as with non-media operation by andrewsg · Pull Request #447 · googleapis/python-storage

andrewsg · 2021-05-20T23:47:38Z

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.

andrewsg · 2021-05-20T23:48:14Z

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.

frankyn · 2021-05-24T17:15:03Z

google/cloud/storage/_helpers.py

+        raise ValueError("num_retries and retry arguments are mutually exclusive")
+
+    elif retry is not None:
+        return resumable_media.RetryStrategy(


It looks like error types are not translatable in this case?

You mean custom predicates? Yes, those are not translatable. It'll be documented wherever possible.

frankyn · 2021-05-24T17:16:08Z

google/cloud/storage/blob.py

                end=end,
                checksum=checksum,
            )
+            download._retry_strategy = retry_strategy


IIUC, this is accessing a private variable maiintained in ResumableMedia, should resumable media expose a mutable setting instead?

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.

frankyn · 2021-05-24T17:19:02Z

google/cloud/storage/blob.py

+            to configure them.
        """
+
+        retry_strategy = _api_core_retry_to_resumable_media_retry(retry)


Would it be reasonable to expose a configurable option for mutating which errors are retried in ResumableMedia package or a pass in a lambda?

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.

tritone · 2021-05-24T17:26:16Z

google/cloud/storage/_helpers.py

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


Is this clear to the end user?

I'll have this warning in every single public method docstring, so with that it should be clear.

tritone · 2021-05-24T17:27:25Z

google/cloud/storage/blob.py

+
+        :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,


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?

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.

tritone · 2021-05-24T17:36:01Z

google/cloud/storage/blob.py

+            # arguments into query_params dictionaries. Media operations work
+            # differently, so here we make a "fake" query_params to feed to the
+            # ConditionalRetryPolicy.
+            query_params = {


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.

They will be None. Optional in the docstring means it may be either the declared type or None.

cojenco · 2021-06-10T00:33:24Z

google/cloud/storage/blob.py

-            num_retries = 0
+
+        # Handle ConditionalRetryPolicy.
+        if isinstance(retry, ConditionalRetryPolicy):


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?

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.

snippet-bot · 2021-06-11T00:55:47Z

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

andrewsg · 2021-06-11T01:33:54Z

Currently working through some absolutely brutal test merge conflicts, will ask for further review when done.

andrewsg · 2021-06-11T02:39:29Z

@frankyn @tritone @cojenco PTAL. Docstrings added, all comments responded to, merge conflict resolved.

@tseaver As discussed in chat, also PTAL due to significant potential for error on my part in resolving blob method -> client method-related merge conflicts.

google/cloud/storage/client.py

tests/unit/test_client.py

tests/unit/test__helpers.py

tests/unit/test_client.py

tests/unit/test_fileio.py

andrewsg · 2021-06-11T17:32:33Z

@tseaver PTAL. Thank you for your attention, I have much more confidence in the merge now.

frankyn

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.

andrewsg added 6 commits March 30, 2021 11:25

fix: Ensure consistency check in test runs even if expected error occurs

9a0eede

tests pass

f43f630

tests pass with refactor

0d3d260

version bump for dependencies; tests pass for real now

265d3bc

complete tests for download retries

a820538

coverage and test

20afb8e

andrewsg requested review from a team, cojenco, frankyn and tseaver May 20, 2021 23:47

andrewsg requested a review from a team as a code owner May 20, 2021 23:47

product-auto-label bot added the api: storage Issues related to the googleapis/python-storage API. label May 20, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 20, 2021

andrewsg added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 20, 2021

frankyn requested a review from tritone May 21, 2021 17:14

frankyn suggested changes May 24, 2021

View reviewed changes

tritone reviewed May 24, 2021

View reviewed changes

frankyn self-requested a review June 9, 2021 18:13

cojenco reviewed Jun 10, 2021

View reviewed changes

andrewsg removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 11, 2021

Finish coverage and add docstrings

4b37672

andrewsg force-pushed the retry-unification branch from 3caedef to 4b37672 Compare June 11, 2021 00:57

very challenging merge

c051371

tseaver reviewed Jun 11, 2021

View reviewed changes

address tseaver comments

39f03b1

tseaver approved these changes Jun 11, 2021

View reviewed changes

tseaver mentioned this pull request Jun 11, 2021

docs: streamline docstrings for conditional parmas #464

Merged

frankyn approved these changes Jun 11, 2021

View reviewed changes

andrewsg merged commit 0dbbb8a into master Jun 11, 2021

andrewsg deleted the retry-unification branch June 11, 2021 22:26

cojenco mentioned this pull request Jun 29, 2021

feat: add preconditions and retry configuration to blob.create_resumable_upload_session #484

Merged

tswast mentioned this pull request Nov 18, 2021

Remove num_retries parameter from load_table_from_*() methods googleapis/python-bigquery#2397

Open

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

chore(main): release 3.0.0 #688

Closed

Conversation

andrewsg commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewsg commented May 20, 2021

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!

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!

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!

snippet-bot bot commented Jun 11, 2021

No region tags are edited in this PR.

Uh oh!

andrewsg commented Jun 11, 2021

Uh oh!

andrewsg commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewsg commented Jun 11, 2021

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andrewsg commented May 20, 2021 •

edited

Loading

andrewsg commented Jun 11, 2021 •

edited

Loading