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

Updates for retry documentation for cloud storage #658

Closed
sev-fletch opened this issue Dec 1, 2021 · 1 comment · Fixed by #808
Closed

Updates for retry documentation for cloud storage #658

sev-fletch opened this issue Dec 1, 2021 · 1 comment · Fixed by #808
Assignees
Labels
api: storage Issues related to the googleapis/python-storage API. priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. samples Issues that are directly related to samples. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@sev-fletch
Copy link

I was looking at this doc on how to use Retry and ConditionalRetryPolicy. Two things I came across:

  • I wasn't able to get the ConditionalRetryPolicy working given the sample code
  • It would help to surface this comment in the Retry example

For ConditionalRetryPolicy this is the example code:

from google.cloud.storage.retry import ConditionalRetryPolicy
from google.cloud.storage.retry import is_etag_in_data

def is_retryable(exc):
    ... # as above

my_retry_policy = Retry(predicate=is_retryable)
my_cond_policy = ConditionalRetryPolicy(
    my_retry_policy, conditional_predicate=is_etag_in_data)
bucket = client.get_bucket(BUCKET_NAME, retry=my_cond_policy)

Looking at the source code there is an additional parameter required_kwargs for ConditionalRetryPolicy that is missing from the example. It should be:

my_cond_policy = ConditionalRetryPolicy(
    my_retry_policy, is_etag_in_data, ["query_params"])

The documentation below this example mentions the required_kwargs parameter but not in a way I was able to make sense of as an end user:

required_kwargs (list(str)) – A list of keyword argument keys that will be extracted from the API call and passed into the conditional predicate in order.

Since it seems like the required_kwargs should always be ['query_params'] from the client PoV it might make sense to set this as a default, or update the documentation to explicitly mention this value.

For Retry - if its intended that end users can use DEFAULT_RETRY as in this comment it would be hepful to see that alongside the current Retry example:

_MY_RETRIABLE_TYPES = [
   exceptions.TooManyRequests,  # 429
   exceptions.InternalServerError,  # 500
   exceptions.BadGateway,  # 502
   exceptions.ServiceUnavailable,  # 503
]

def is_retryable(exc):
    return isinstance(exc, _MY_RETRIABLE_TYPES)

my_retry_policy = Retry(predicate=is_retryable)
bucket = client.get_bucket(BUCKET_NAME, retry=my_retry_policy)

OR

from google.cloud.storage.retry import DEFAULT_RETRY

my_retry_policy = DEFAULT_RETRY.with_deadline(30)
bucket = client.get_bucket(BUCKET_NAME, retry=my_retry_policy)

Thanks!

@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/python-storage API. label Dec 1, 2021
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Dec 2, 2021
@andrewsg andrewsg self-assigned this Dec 3, 2021
@andrewsg andrewsg added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. samples Issues that are directly related to samples. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Dec 3, 2021
@andrewsg
Copy link
Contributor

andrewsg commented Dec 3, 2021

Thank you for this report and your suggestions!

@yoshi-automation yoshi-automation removed the triage me I really want to be triaged. label Dec 3, 2021
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Mar 3, 2022
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 2, 2022
@cojenco cojenco assigned cojenco and unassigned andrewsg Jun 3, 2022
@cojenco cojenco removed the 🚨 This issue needs some love. label Jun 3, 2022
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 3, 2022
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. priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. samples Issues that are directly related to samples. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants