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
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
d0af8e3
feat: make retry parameter public and added in other methods
HemangChothani 4424b59
feat: resolve conflict
HemangChothani 674b0f2
feat: change in doc string
HemangChothani 3d36abb
Merge branch 'master' of https://github.com/googleapis/python-storage…
HemangChothani e204297
feat: changes in docstring
HemangChothani 71bdd7e
feat: remove retry for acl
HemangChothani File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,7 @@ | |
""" | ||
|
||
from google.cloud.storage.constants import _DEFAULT_TIMEOUT | ||
from google.cloud.storage.retry import DEFAULT_RETRY | ||
|
||
|
||
class _ACLEntity(object): | ||
|
@@ -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): | ||
"""Reload the ACL data from Cloud Storage. | ||
|
||
If :attr:`user_project` is set, bills the API request to that project. | ||
|
@@ -445,6 +446,20 @@ def reload(self, client=None, timeout=_DEFAULT_TIMEOUT): | |
|
||
Can also be passed as a tuple (connect_timeout, read_timeout). | ||
See :meth:`requests.Session.request` documentation for details. | ||
|
||
:type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy | ||
:param retry: (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. | ||
""" | ||
path = self.reload_path | ||
client = self._require_client(client) | ||
|
@@ -456,13 +471,19 @@ def reload(self, client=None, timeout=_DEFAULT_TIMEOUT): | |
self.entities.clear() | ||
|
||
found = client._connection.api_request( | ||
method="GET", path=path, query_params=query_params, timeout=timeout | ||
method="GET", | ||
path=path, | ||
query_params=query_params, | ||
timeout=timeout, | ||
retry=retry, | ||
) | ||
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. Choose a reason for hiding this commentThe 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! |
||
self, acl, predefined, client, timeout=_DEFAULT_TIMEOUT, retry=DEFAULT_RETRY | ||
): | ||
"""Helper for :meth:`save` and :meth:`save_predefined`. | ||
|
||
:type acl: :class:`google.cloud.storage.acl.ACL`, or a compatible list. | ||
|
@@ -483,6 +504,20 @@ def _save(self, acl, predefined, client, timeout=_DEFAULT_TIMEOUT): | |
|
||
Can also be passed as a tuple (connect_timeout, read_timeout). | ||
See :meth:`requests.Session.request` documentation for details. | ||
|
||
:type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy | ||
:param retry: (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. | ||
""" | ||
query_params = {"projection": "full"} | ||
if predefined is not None: | ||
|
@@ -501,13 +536,16 @@ def _save(self, acl, predefined, client, timeout=_DEFAULT_TIMEOUT): | |
data={self._URL_PATH_ELEM: list(acl)}, | ||
query_params=query_params, | ||
timeout=timeout, | ||
retry=retry, | ||
) | ||
self.entities.clear() | ||
for entry in result.get(self._URL_PATH_ELEM, ()): | ||
self.add_entity(self.entity_from_dict(entry)) | ||
self.loaded = True | ||
|
||
def save(self, acl=None, client=None, timeout=_DEFAULT_TIMEOUT): | ||
def save( | ||
self, acl=None, client=None, timeout=_DEFAULT_TIMEOUT, retry=DEFAULT_RETRY | ||
): | ||
"""Save this ACL for the current bucket. | ||
|
||
If :attr:`user_project` is set, bills the API request to that project. | ||
|
@@ -526,6 +564,20 @@ def save(self, acl=None, client=None, timeout=_DEFAULT_TIMEOUT): | |
|
||
Can also be passed as a tuple (connect_timeout, read_timeout). | ||
See :meth:`requests.Session.request` documentation for details. | ||
|
||
:type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy | ||
:param retry: (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. | ||
""" | ||
if acl is None: | ||
acl = self | ||
|
@@ -534,9 +586,11 @@ def save(self, acl=None, client=None, timeout=_DEFAULT_TIMEOUT): | |
save_to_backend = True | ||
|
||
if save_to_backend: | ||
self._save(acl, None, client, timeout=timeout) | ||
self._save(acl, None, client, timeout=timeout, retry=retry) | ||
|
||
def save_predefined(self, predefined, client=None, timeout=_DEFAULT_TIMEOUT): | ||
def save_predefined( | ||
self, predefined, client=None, timeout=_DEFAULT_TIMEOUT, retry=DEFAULT_RETRY | ||
): | ||
"""Save this ACL for the current bucket using a predefined ACL. | ||
|
||
If :attr:`user_project` is set, bills the API request to that project. | ||
|
@@ -558,11 +612,25 @@ def save_predefined(self, predefined, client=None, timeout=_DEFAULT_TIMEOUT): | |
|
||
Can also be passed as a tuple (connect_timeout, read_timeout). | ||
See :meth:`requests.Session.request` documentation for details. | ||
|
||
:type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy | ||
:param retry: (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. | ||
""" | ||
predefined = self.validate_predefined(predefined) | ||
self._save(None, predefined, client, timeout=timeout) | ||
self._save(None, predefined, client, timeout=timeout, retry=retry) | ||
|
||
def clear(self, client=None, timeout=_DEFAULT_TIMEOUT): | ||
def clear(self, client=None, timeout=_DEFAULT_TIMEOUT, retry=DEFAULT_RETRY): | ||
"""Remove all ACL entries. | ||
|
||
If :attr:`user_project` is set, bills the API request to that project. | ||
|
@@ -582,8 +650,22 @@ def clear(self, client=None, timeout=_DEFAULT_TIMEOUT): | |
|
||
Can also be passed as a tuple (connect_timeout, read_timeout). | ||
See :meth:`requests.Session.request` documentation for details. | ||
|
||
:type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy | ||
:param retry: (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.save([], client=client, timeout=timeout) | ||
self.save([], client=client, timeout=timeout, retry=retry) | ||
|
||
|
||
class BucketACL(ACL): | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
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.