Skip to content
Permalink
Browse files
fix: retry uploads only conditionally (#316)
* fix: retry uploads only conditionally

* update docstring for num_retries
  • Loading branch information
andrewsg committed Nov 20, 2020
1 parent dc36719 commit 547740c0a898492e76ce5e60dd20c7ddb8a53d1f
Showing with 60 additions and 13 deletions.
  1. +54 −13 google/cloud/storage/blob.py
  2. +6 −0 tests/unit/test_blob.py
@@ -103,9 +103,11 @@
"release. The default behavior (when `num_retries` is not specified) when "
"a transient error (e.g. 429 Too Many Requests or 500 Internal Server "
"Error) occurs will be as follows: upload requests will be automatically "
"retried. Subsequent retries will be sent after waiting 1, 2, 4, 8, etc. "
"seconds (exponential backoff) until 10 minutes of wait time have "
"elapsed. At that point, there will be no more attempts to retry."
"retried if and only if `if_metageneration_match` is specified (thus "
"making the upload idempotent). Subsequent retries will be sent after "
"waiting 1, 2, 4, 8, etc. seconds (exponential backoff) until 10 minutes "
"of wait time have elapsed. At that point, there will be no more attempts "
"to retry."
)
_READ_LESS_THAN_SIZE = (
"Size {:d} was specified but the file-like object only had " "{:d} bytes remaining."
@@ -1550,8 +1552,14 @@ def _do_multipart_upload(
concluded once ``stream`` is exhausted (or :data:`None`).
:type num_retries: int
:param num_retries: Number of upload retries. (Deprecated: This
argument will be removed in a future release.)
:param num_retries: Number of upload retries. By default, only uploads
with if_metageneration_match set will be retried, as
uploads without the argument are not guaranteed to
be idempotent. Setting num_retries will override
this default behavior and guarantee retries even
when if_metageneration_match is not set.
(Deprecated: This argument will be removed in a
future release.)
:type predefined_acl: str
:param predefined_acl: (Optional) Predefined access control list
@@ -1709,8 +1717,14 @@ def _initiate_resumable_upload(
:param predefined_acl: (Optional) Predefined access control list
:type num_retries: int
:param num_retries: Number of upload retries. (Deprecated: This
argument will be removed in a future release.)
:param num_retries: Number of upload retries. By default, only uploads
with if_metageneration_match set will be retried, as
uploads without the argument are not guaranteed to
be idempotent. Setting num_retries will override
this default behavior and guarantee retries even
when if_metageneration_match is not set.
(Deprecated: This argument will be removed in a
future release.)
:type extra_headers: dict
:param extra_headers: (Optional) Extra headers to add to standard
@@ -1887,8 +1901,14 @@ def _do_resumable_upload(
concluded once ``stream`` is exhausted (or :data:`None`).
:type num_retries: int
:param num_retries: Number of upload retries. (Deprecated: This
argument will be removed in a future release.)
:param num_retries: Number of upload retries. By default, only uploads
with if_metageneration_match set will be retried, as
uploads without the argument are not guaranteed to
be idempotent. Setting num_retries will override
this default behavior and guarantee retries even
when if_metageneration_match is not set.
(Deprecated: This argument will be removed in a
future release.)
:type predefined_acl: str
:param predefined_acl: (Optional) Predefined access control list
@@ -2005,8 +2025,14 @@ def _do_upload(
concluded once ``stream`` is exhausted (or :data:`None`).
:type num_retries: int
:param num_retries: Number of upload retries. (Deprecated: This
argument will be removed in a future release.)
:param num_retries: Number of upload retries. By default, only uploads
with if_metageneration_match set will be retried, as
uploads without the argument are not guaranteed to
be idempotent. Setting num_retries will override
this default behavior and guarantee retries even
when if_metageneration_match is not set.
(Deprecated: This argument will be removed in a
future release.)
:type predefined_acl: str
:param predefined_acl: (Optional) Predefined access control list
@@ -2058,6 +2084,15 @@ def _do_upload(
**only** response in the multipart case and it will be the
**final** response in the resumable case.
"""
if if_metageneration_match is None and num_retries is None:
# Uploads are only idempotent (safe to retry) if
# if_metageneration_match is set. If it is not set, the default
# num_retries should be 0. Note: Because retry logic for uploads is
# provided by the google-resumable-media-python package, it doesn't
# use the ConditionalRetryStrategy class used in other API calls in
# this library to solve this problem.
num_retries = 0

if size is not None and size <= _MAX_MULTIPART_SIZE:
response = self._do_multipart_upload(
client,
@@ -2161,8 +2196,14 @@ def upload_from_file(
:param content_type: (Optional) Type of content being uploaded.
:type num_retries: int
:param num_retries: Number of upload retries. (Deprecated: This
argument will be removed in a future release.)
:param num_retries: Number of upload retries. By default, only uploads
with if_metageneration_match set will be retried, as
uploads without the argument are not guaranteed to
be idempotent. Setting num_retries will override
this default behavior and guarantee retries even
when if_metageneration_match is not set.
(Deprecated: This argument will be removed in a
future release.)
:type client: :class:`~google.cloud.storage.client.Client`
:param client: (Optional) The client to use. If not passed, falls back
@@ -2577,6 +2577,12 @@ def _do_upload_helper(
if_metageneration_not_match,
**timeout_kwarg
)

# Adjust num_retries expectations to reflect the conditional default in
# _do_upload()
if num_retries is None and if_metageneration_match is None:
num_retries = 0

self.assertIs(created_json, mock.sentinel.json)
response.json.assert_called_once_with()
if size is not None and size <= _MAX_MULTIPART_SIZE:

0 comments on commit 547740c

Please sign in to comment.