From 547740c0a898492e76ce5e60dd20c7ddb8a53d1f Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Fri, 20 Nov 2020 09:38:07 -0800 Subject: [PATCH] fix: retry uploads only conditionally (#316) * fix: retry uploads only conditionally * update docstring for num_retries --- google/cloud/storage/blob.py | 67 +++++++++++++++++++++++++++++------- tests/unit/test_blob.py | 6 ++++ 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index e6b79dab5..18006d5ad 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/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 diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index 87c2a4878..bc9918627 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -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: