Skip to content
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: make retry parameter public and added in other methods #331

Merged
merged 6 commits into from
Dec 9, 2020

Conversation

HemangChothani
Copy link
Contributor

Fixes #315

@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/python-storage API. label Dec 1, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 1, 2020
@frankyn frankyn requested review from andrewsg and removed request for frankyn December 1, 2020 19:55
@andrewsg
Copy link
Contributor

andrewsg commented Dec 1, 2020

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.

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@HemangChothani HemangChothani requested a review from a team December 3, 2020 13:12
@@ -190,7 +190,9 @@ def reload(
blob's current metageneration does not match the given value.

:type retry: google.api_core.retry.Retry
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

: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.
Copy link
Contributor

@andrewsg andrewsg Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)
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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

@andrewsg andrewsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for your efforts here!

@andrewsg
Copy link
Contributor

andrewsg commented Dec 9, 2020

@HemangChothani are we clear to merge?

@HemangChothani
Copy link
Contributor Author

@andrewsg Yes.

@andrewsg andrewsg merged commit 910e34c into googleapis:master Dec 9, 2020
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
…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
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/python-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a default retry parameter to all public methods
3 participants