Skip to content
Permalink
Browse files
fix: replace default retry for upload operations (#480)
* fix: revise upload operations preconditions to if_generation_match

* fix docstrings

* add retry configuration to blob.create_resumable_upload_session

* align var values in test

* test coverage

* Revert "test coverage"

This reverts commit e91916f.

* Revert "align var values in test"

This reverts commit aec585b.

* Revert "add retry configuration to blob.create_resumable_upload_session"

This reverts commit 8c1ae3c.

* revise tests after reverting
  • Loading branch information
cojenco committed Jun 30, 2021
1 parent e3e57a9 commit c027ccf4279fb05e041754294f10744b7d81beea
Showing with 36 additions and 40 deletions.
  1. +25 −26 google/cloud/storage/blob.py
  2. +3 −3 google/cloud/storage/fileio.py
  3. +6 −9 tests/unit/test_blob.py
  4. +2 −2 tests/unit/test_fileio.py
@@ -82,7 +82,6 @@
from google.cloud.storage.retry import DEFAULT_RETRY
from google.cloud.storage.retry import DEFAULT_RETRY_IF_ETAG_IN_JSON
from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED
from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED
from google.cloud.storage.fileio import BlobReader
from google.cloud.storage.fileio import BlobWriter

@@ -1703,10 +1702,10 @@ def _do_multipart_upload(
:type num_retries: int
:param num_retries:
Number of upload retries. By default, only uploads with
if_metageneration_match set will be retried, as uploads without the
if_generation_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
if_generation_match is not set. (Deprecated: This argument
will be removed in a future release.)
:type predefined_acl: str
@@ -1750,7 +1749,7 @@ def _do_multipart_upload(
This private method does not accept ConditionalRetryPolicy values
because the information necessary to evaluate the policy is instead
evaluated in client.download_blob_to_file().
evaluated in blob._do_upload().
See the retry.py source code and docstrings in this package
(google.cloud.storage.retry) for information on retry types and how
@@ -1877,10 +1876,10 @@ def _initiate_resumable_upload(
:type num_retries: int
:param num_retries:
Number of upload retries. By default, only uploads with
if_metageneration_match set will be retried, as uploads without the
if_generation_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
if_generation_match is not set. (Deprecated: This argument
will be removed in a future release.)
:type extra_headers: dict
@@ -1936,7 +1935,7 @@ def _initiate_resumable_upload(
This private method does not accept ConditionalRetryPolicy values
because the information necessary to evaluate the policy is instead
evaluated in client.download_blob_to_file().
evaluated in blob._do_upload().
See the retry.py source code and docstrings in this package
(google.cloud.storage.retry) for information on retry types and how
@@ -2070,10 +2069,10 @@ def _do_resumable_upload(
:type num_retries: int
:param num_retries:
Number of upload retries. By default, only uploads with
if_metageneration_match set will be retried, as uploads without the
if_generation_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
if_generation_match is not set. (Deprecated: This argument
will be removed in a future release.)
:type predefined_acl: str
@@ -2119,7 +2118,7 @@ def _do_resumable_upload(
This private method does not accept ConditionalRetryPolicy values
because the information necessary to evaluate the policy is instead
evaluated in client.download_blob_to_file().
evaluated in blob._do_upload().
See the retry.py source code and docstrings in this package
(google.cloud.storage.retry) for information on retry types and how
@@ -2204,10 +2203,10 @@ def _do_upload(
:type num_retries: int
:param num_retries:
Number of upload retries. By default, only uploads with
if_metageneration_match set will be retried, as uploads without the
if_generation_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
if_generation_match is not set. (Deprecated: This argument
will be removed in a future release.)
:type predefined_acl: str
@@ -2258,7 +2257,7 @@ def _do_upload(
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.
condition such as if_generation_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
@@ -2337,7 +2336,7 @@ def upload_from_file(
if_metageneration_not_match=None,
timeout=_DEFAULT_TIMEOUT,
checksum=None,
retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
):
"""Upload the contents of this blob from a file-like object.
@@ -2397,10 +2396,10 @@ def upload_from_file(
:type num_retries: int
:param num_retries:
Number of upload retries. By default, only uploads with
if_metageneration_match set will be retried, as uploads without the
if_generation_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
if_generation_match is not set. (Deprecated: This argument
will be removed in a future release.)
:type client: :class:`~google.cloud.storage.client.Client`
@@ -2456,7 +2455,7 @@ def upload_from_file(
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.
condition such as if_generation_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
@@ -2479,7 +2478,7 @@ def upload_from_file(
# num_retries and retry are mutually exclusive. If num_retries is
# set and retry is exactly the default, then nullify retry for
# backwards compatibility.
if retry is DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED:
if retry is DEFAULT_RETRY_IF_GENERATION_SPECIFIED:
retry = None

_maybe_rewind(file_obj, rewind=rewind)
@@ -2518,7 +2517,7 @@ def upload_from_filename(
if_metageneration_not_match=None,
timeout=_DEFAULT_TIMEOUT,
checksum=None,
retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
):
"""Upload this blob's contents from the content of a named file.
@@ -2558,10 +2557,10 @@ def upload_from_filename(
:type num_retries: int
:param num_retries:
Number of upload retries. By default, only uploads with
if_metageneration_match set will be retried, as uploads without the
if_generation_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
if_generation_match is not set. (Deprecated: This argument
will be removed in a future release.)
:type predefined_acl: str
@@ -2612,7 +2611,7 @@ def upload_from_filename(
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.
condition such as if_generation_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
@@ -2656,7 +2655,7 @@ def upload_from_string(
if_metageneration_not_match=None,
timeout=_DEFAULT_TIMEOUT,
checksum=None,
retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
):
"""Upload contents of this blob from the provided string.
@@ -2687,10 +2686,10 @@ def upload_from_string(
:type num_retries: int
:param num_retries:
Number of upload retries. By default, only uploads with
if_metageneration_match set will be retried, as uploads without the
if_generation_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
if_generation_match is not set. (Deprecated: This argument
will be removed in a future release.)
:type client: :class:`~google.cloud.storage.client.Client`
@@ -2746,7 +2745,7 @@ def upload_from_string(
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.
condition such as if_generation_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
@@ -18,7 +18,7 @@
from google.api_core.exceptions import RequestRangeNotSatisfiable
from google.cloud.storage._helpers import _NUM_RETRIES_MESSAGE
from google.cloud.storage.retry import DEFAULT_RETRY
from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED
from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED
from google.cloud.storage.retry import ConditionalRetryPolicy


@@ -278,7 +278,7 @@ def __init__(
blob,
chunk_size=None,
text_mode=False,
retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
**upload_kwargs
):
for kwarg in upload_kwargs:
@@ -346,7 +346,7 @@ def _initiate_upload(self):
# num_retries and retry are mutually exclusive. If num_retries is
# set and retry is exactly the default, then nullify retry for
# backwards compatibility.
if retry is DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED:
if retry is DEFAULT_RETRY_IF_GENERATION_SPECIFIED:
retry = None

# Handle ConditionalRetryPolicy.
@@ -29,7 +29,6 @@
from google.cloud.storage.retry import DEFAULT_RETRY
from google.cloud.storage.retry import DEFAULT_RETRY_IF_ETAG_IN_JSON
from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED
from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED


def _make_credentials():
@@ -2853,8 +2852,8 @@ def _do_upload_helper(
**timeout_kwarg
)

if retry is DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED:
retry = DEFAULT_RETRY if if_metageneration_match else None
if retry is DEFAULT_RETRY_IF_GENERATION_SPECIFIED:
retry = DEFAULT_RETRY if if_generation_match else None

self.assertIs(created_json, mock.sentinel.json)
response.json.assert_called_once_with()
@@ -2925,11 +2924,11 @@ def test__do_upload_with_num_retries(self):

def test__do_upload_with_conditional_retry_success(self):
self._do_upload_helper(
retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, if_metageneration_match=1
retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, if_generation_match=123456
)

def test__do_upload_with_conditional_retry_failure(self):
self._do_upload_helper(retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED)
self._do_upload_helper(retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED)

def _upload_from_file_helper(self, side_effect=None, **kwargs):
from google.cloud._helpers import UTC
@@ -2955,7 +2954,7 @@ def _upload_from_file_helper(self, side_effect=None, **kwargs):
if_metageneration_not_match = kwargs.get("if_metageneration_not_match", None)
num_retries = kwargs.get("num_retries", None)
default_retry = (
DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED if not num_retries else None
DEFAULT_RETRY_IF_GENERATION_SPECIFIED if not num_retries else None
)
retry = kwargs.get("retry", default_retry)
ret_val = blob.upload_from_file(
@@ -3062,9 +3061,7 @@ def _do_upload_mock_call_helper(

expected_timeout = self._get_default_timeout() if timeout is None else timeout
if not retry:
retry = (
DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED if not num_retries else None
)
retry = DEFAULT_RETRY_IF_GENERATION_SPECIFIED if not num_retries else None
self.assertEqual(
kwargs, {"timeout": expected_timeout, "checksum": None, "retry": retry}
)
@@ -395,7 +395,7 @@ def test_conditional_retry_pass(self):
blob,
chunk_size=chunk_size,
content_type=PLAIN_CONTENT_TYPE,
if_metageneration_match=1,
if_generation_match=123456,
)

# The transmit_next_chunk method must actually consume bytes from the
@@ -421,7 +421,7 @@ def test_conditional_retry_pass(self):
None, # num_retries
chunk_size=chunk_size,
retry=DEFAULT_RETRY,
if_metageneration_match=1,
if_generation_match=123456,
)
upload.transmit_next_chunk.assert_called_with(transport)
self.assertEqual(upload.transmit_next_chunk.call_count, 4)

0 comments on commit c027ccf

Please sign in to comment.