feat: make retry parameter public and added in other methods #331
Conversation
product-auto-label
bot
added
the
api: storage
google-cla
bot
added
the
cla: yes
|
Thanks! This is a major change to the UI in line with our medium-term plan for retry support, so I (author of underlying retry logic) will review thoroughly soon. |
google/cloud/storage/_helpers.py
Outdated
| @@ -187,6 +188,9 @@ def reload( | |||
| :type if_metageneration_not_match: long | |||
| :param if_metageneration_not_match: (Optional) Make the operation conditional on whether the | |||
| blob's current metageneration does not match the given value. | |||
|
|
|||
| :type retry: google.api_core.retry.Retry | |||
| :param retry: (Optional) How to retry the RPC. | |||
There was a problem hiding this comment.
In order for users to use this, they'll need an explanation of what kinds of objects to pass in and how to modify them. "See retry.py" might be enough if retry.py's comments and docstrings are comprehensive.
google/cloud/storage/acl.py
Outdated
| @@ -430,7 +431,7 @@ def _require_client(self, client): | |||
| client = self.client | |||
| return client | |||
|
|
|||
| def reload(self, client=None, timeout=_DEFAULT_TIMEOUT): | |||
| def reload(self, client=None, timeout=_DEFAULT_TIMEOUT, retry=DEFAULT_RETRY): | |||
There was a problem hiding this comment.
Here in the ACL file you're not just exposing retries in the function signature but actually adding a new default, right? @frankyn ACL commands aren't in the consolidated retry strategy doc, do we want retries enabled on them?
There was a problem hiding this comment.
ACLs don't have optimistic concurrency control, therefore we would need to update implementation to rely on storage.objects.patch and storage.buckets.patch instead of ACL specific APIs.
Let's remove ACL retries from this PR for now and open a tracking issue.
…into storage_issue_315
google/cloud/storage/_helpers.py
Outdated
| @@ -190,7 +190,9 @@ def reload( | |||
| blob's current metageneration does not match the given value. | |||
|
|
|||
| :type retry: google.api_core.retry.Retry | |||
There was a problem hiding this comment.
Technically either google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy are supported here, and in some cases ConditionalRetryPolicy is the default so despite the complexity we should document it here.
google/cloud/storage/_helpers.py
Outdated
| :param retry: (Optional) How to retry the RPC. | ||
| :param retry: (Optional) How to retry the RPC. To modify the default retry behavior, | ||
| create a new retry object modeled after this one by calling it a ``with_XXX`` method. | ||
| See: https://googleapis.dev/python/google-api-core/latest/retry.html for details. |
There was a problem hiding this comment.
(Optional) How to retry the RPC. A None value will disable retries. A google.api_core.retry.Retry value will enable retries, and the object will define retriable response codes and errors and configure backoff and timeout options.
A google.cloud.storage.retry.ConditionalRetryPolicy value wraps a Retry object and activates it only if certain conditions are met. This class exists to provide safe defaults for RPC calls that are not technically safe to retry normally (due to potential data duplication or other side-effects) but become safe to retry if a condition such as if_metageneration_match is set.
See the retry.py source code and docstrings in this package (google.cloud.storage.retry) for information on retry types and how to configure them.
google/cloud/storage/acl.py
Outdated
| ) | ||
| self.loaded = True | ||
| for entry in found.get("items", ()): | ||
| self.add_entity(self.entity_from_dict(entry)) | ||
|
|
||
| def _save(self, acl, predefined, client, timeout=_DEFAULT_TIMEOUT): | ||
| def _save( |
There was a problem hiding this comment.
We've investigated and found that ACL save retries don't support the concurrency control we'd need to have full faith in retries here. Please go ahead and remove them. Thanks!
|
@HemangChothani are we clear to merge? |
|
@andrewsg Yes. |
…pis#331) * feat: make retry parameter public and added in other methods * feat: change in doc string * feat: changes in docstring * feat: remove retry for acl
…pis#331) * feat: make retry parameter public and added in other methods * feat: change in doc string * feat: changes in docstring * feat: remove retry for acl
Fixes #315