From 5e7d800658d5c6df8263430e042ecbca5d971051 Mon Sep 17 00:00:00 2001 From: Sangram Date: Sun, 14 Jul 2019 19:14:58 +0530 Subject: [PATCH 1/7] Add Truncated exponential backoff for connection reset --- storage/google/cloud/storage/_helpers.py | 30 ++++++++++++++++++------ 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/storage/google/cloud/storage/_helpers.py b/storage/google/cloud/storage/_helpers.py index 93848daa1cde..b982d7882c46 100644 --- a/storage/google/cloud/storage/_helpers.py +++ b/storage/google/cloud/storage/_helpers.py @@ -52,6 +52,8 @@ class _PropertyMixin(object): number or letter. """ + retries = 4 # number of retries on connection failure + def __init__(self, name=None): self.name = name self._properties = {} @@ -117,18 +119,32 @@ def reload(self, client=None): :param client: the client to use. If not passed, falls back to the ``client`` stored on the current object. """ + import time + import random + client = self._require_client(client) query_params = self._query_params # Pass only '?projection=noAcl' here because 'acl' and related # are handled via custom endpoints. query_params["projection"] = "noAcl" - api_response = client._connection.api_request( - method="GET", - path=self.path, - query_params=query_params, - headers=self._encryption_headers(), - _target_object=self, - ) + + for retry in range(self.retries): + try: + time.sleep(min(random.random() + 2 ** (retry - 1), 32)) + api_response = client._connection.api_request( + method="GET", + path=self.path, + query_params=query_params, + headers=self._encryption_headers(), + _target_object=self, + ) + break + except ConnectionError: + if retry == self.retries - 1: + raise ConnectionError + else: + pass + self._set_properties(api_response) def _patch_property(self, name, value): From a2097d70cf1c11d2571211612f42a5c676a93bb1 Mon Sep 17 00:00:00 2001 From: Sangram Date: Wed, 17 Jul 2019 02:56:16 +0530 Subject: [PATCH 2/7] Move retry code to bucket.py, unit testing is implicitly covered and tested. --- storage/google/cloud/storage/_helpers.py | 28 ++++++----------------- storage/google/cloud/storage/bucket.py | 29 ++++++++++++++++-------- storage/tests/unit/test_bucket.py | 2 +- 3 files changed, 28 insertions(+), 31 deletions(-) diff --git a/storage/google/cloud/storage/_helpers.py b/storage/google/cloud/storage/_helpers.py index b982d7882c46..35c80bf85d4b 100644 --- a/storage/google/cloud/storage/_helpers.py +++ b/storage/google/cloud/storage/_helpers.py @@ -52,8 +52,6 @@ class _PropertyMixin(object): number or letter. """ - retries = 4 # number of retries on connection failure - def __init__(self, name=None): self.name = name self._properties = {} @@ -119,31 +117,19 @@ def reload(self, client=None): :param client: the client to use. If not passed, falls back to the ``client`` stored on the current object. """ - import time - import random client = self._require_client(client) query_params = self._query_params # Pass only '?projection=noAcl' here because 'acl' and related # are handled via custom endpoints. query_params["projection"] = "noAcl" - - for retry in range(self.retries): - try: - time.sleep(min(random.random() + 2 ** (retry - 1), 32)) - api_response = client._connection.api_request( - method="GET", - path=self.path, - query_params=query_params, - headers=self._encryption_headers(), - _target_object=self, - ) - break - except ConnectionError: - if retry == self.retries - 1: - raise ConnectionError - else: - pass + api_response = client._connection.api_request( + method="GET", + path=self.path, + query_params=query_params, + headers=self._encryption_headers(), + _target_object=self, + ) self._set_properties(api_response) diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 3c3afbe97af7..32a9e89c5cec 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -459,6 +459,9 @@ class Bucket(_PropertyMixin): ) """Allowed values for :attr:`location_type`.""" + RETRIES = 4 + """Number of retries on connection failure. """ + def __init__(self, client, name=None, user_project=None): name = _validate_name(name) super(Bucket, self).__init__(name=name) @@ -748,6 +751,9 @@ def get_blob( :rtype: :class:`google.cloud.storage.blob.Blob` or None :returns: The blob object if it exists, otherwise None. """ + import time + import random + blob = Blob( bucket=self, name=blob_name, @@ -755,15 +761,20 @@ def get_blob( generation=generation, **kwargs ) - try: - # NOTE: This will not fail immediately in a batch. However, when - # Batch.finish() is called, the resulting `NotFound` will be - # raised. - blob.reload(client=client) - except NotFound: - return None - else: - return blob + for retry in range(self.RETRIES): + try: + # NOTE: This will not fail immediately in a batch. However, when + # Batch.finish() is called, the resulting `NotFound` will be + # raised. + blob.reload(client=client) + break + except Exception: + if retry == self.RETRIES - 1: + return None + else: + rand_num = random.random() + time.sleep(min(rand_num + 2 ** (retry - 1), rand_num + 32)) + return blob def list_blobs( self, diff --git a/storage/tests/unit/test_bucket.py b/storage/tests/unit/test_bucket.py index 9ac4995525cf..3aeac3b5eb7c 100644 --- a/storage/tests/unit/test_bucket.py +++ b/storage/tests/unit/test_bucket.py @@ -678,7 +678,7 @@ def test_get_blob_miss(self): bucket = self._make_one(name=NAME) result = bucket.get_blob(NONESUCH, client=client) self.assertIsNone(result) - kw, = connection._requested + kw = connection._requested[-1] self.assertEqual(kw["method"], "GET") self.assertEqual(kw["path"], "/b/%s/o/%s" % (NAME, NONESUCH)) From f764b4db66ac2d1f20965633deacc0c07c8f3cad Mon Sep 17 00:00:00 2001 From: Sangram Date: Wed, 17 Jul 2019 02:58:34 +0530 Subject: [PATCH 3/7] _helpers.py need not change. --- storage/google/cloud/storage/_helpers.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/storage/google/cloud/storage/_helpers.py b/storage/google/cloud/storage/_helpers.py index 35c80bf85d4b..93848daa1cde 100644 --- a/storage/google/cloud/storage/_helpers.py +++ b/storage/google/cloud/storage/_helpers.py @@ -117,7 +117,6 @@ def reload(self, client=None): :param client: the client to use. If not passed, falls back to the ``client`` stored on the current object. """ - client = self._require_client(client) query_params = self._query_params # Pass only '?projection=noAcl' here because 'acl' and related @@ -130,7 +129,6 @@ def reload(self, client=None): headers=self._encryption_headers(), _target_object=self, ) - self._set_properties(api_response) def _patch_property(self, name, value): From ef334c047ad0aafd890ee95653d0581084c354be Mon Sep 17 00:00:00 2001 From: Sangram Date: Wed, 17 Jul 2019 03:09:48 +0530 Subject: [PATCH 4/7] Add retry backoff tests --- storage/tests/unit/test_bucket.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/storage/tests/unit/test_bucket.py b/storage/tests/unit/test_bucket.py index 3aeac3b5eb7c..b8607dcaaa67 100644 --- a/storage/tests/unit/test_bucket.py +++ b/storage/tests/unit/test_bucket.py @@ -678,6 +678,8 @@ def test_get_blob_miss(self): bucket = self._make_one(name=NAME) result = bucket.get_blob(NONESUCH, client=client) self.assertIsNone(result) + num_requests = len(connection._requested) + self.assertGreater(num_requests, 1) kw = connection._requested[-1] self.assertEqual(kw["method"], "GET") self.assertEqual(kw["path"], "/b/%s/o/%s" % (NAME, NONESUCH)) From 5f90b958936c1d506d0e42b90eaeb21548f292d1 Mon Sep 17 00:00:00 2001 From: Sangram Date: Fri, 19 Jul 2019 16:53:35 +0530 Subject: [PATCH 5/7] Adding exception assert --- storage/google/cloud/storage/bucket.py | 6 +++--- storage/tests/unit/test_bucket.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 32a9e89c5cec..9f981058600c 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -761,16 +761,16 @@ def get_blob( generation=generation, **kwargs ) - for retry in range(self.RETRIES): + for retry in range(self.RETRIES): # pragma: no branch try: # NOTE: This will not fail immediately in a batch. However, when # Batch.finish() is called, the resulting `NotFound` will be # raised. blob.reload(client=client) break - except Exception: + except Exception as e: if retry == self.RETRIES - 1: - return None + raise e else: rand_num = random.random() time.sleep(min(rand_num + 2 ** (retry - 1), rand_num + 32)) diff --git a/storage/tests/unit/test_bucket.py b/storage/tests/unit/test_bucket.py index b8607dcaaa67..36c445ce715a 100644 --- a/storage/tests/unit/test_bucket.py +++ b/storage/tests/unit/test_bucket.py @@ -676,8 +676,8 @@ def test_get_blob_miss(self): connection = _Connection() client = _Client(connection) bucket = self._make_one(name=NAME) - result = bucket.get_blob(NONESUCH, client=client) - self.assertIsNone(result) + with self.assertRaises(Exception): + result = bucket.get_blob(NONESUCH, client=client) num_requests = len(connection._requested) self.assertGreater(num_requests, 1) kw = connection._requested[-1] From 063523f6a0fc7354ea8c9e994ecaf9cae2c67810 Mon Sep 17 00:00:00 2001 From: Sangram Date: Fri, 19 Jul 2019 17:27:08 +0530 Subject: [PATCH 6/7] Fix lint failures. My bad. --- storage/google/cloud/storage/bucket.py | 2 +- storage/tests/unit/test_bucket.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 9f981058600c..2240c59b2093 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -761,7 +761,7 @@ def get_blob( generation=generation, **kwargs ) - for retry in range(self.RETRIES): # pragma: no branch + for retry in range(self.RETRIES): # pragma: no branch try: # NOTE: This will not fail immediately in a batch. However, when # Batch.finish() is called, the resulting `NotFound` will be diff --git a/storage/tests/unit/test_bucket.py b/storage/tests/unit/test_bucket.py index 36c445ce715a..530cf9203c53 100644 --- a/storage/tests/unit/test_bucket.py +++ b/storage/tests/unit/test_bucket.py @@ -677,7 +677,7 @@ def test_get_blob_miss(self): client = _Client(connection) bucket = self._make_one(name=NAME) with self.assertRaises(Exception): - result = bucket.get_blob(NONESUCH, client=client) + bucket.get_blob(NONESUCH, client=client) num_requests = len(connection._requested) self.assertGreater(num_requests, 1) kw = connection._requested[-1] From e0379853508ff57f93110ced3d695fa1e794f468 Mon Sep 17 00:00:00 2001 From: Sangram Date: Mon, 12 Aug 2019 23:01:28 +0530 Subject: [PATCH 7/7] Review changes --- storage/google/cloud/storage/bucket.py | 10 ++++++---- storage/tests/unit/test_bucket.py | 5 ++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 2240c59b2093..35563ca181dd 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -19,6 +19,7 @@ import datetime import json import warnings +import time import six @@ -751,7 +752,6 @@ def get_blob( :rtype: :class:`google.cloud.storage.blob.Blob` or None :returns: The blob object if it exists, otherwise None. """ - import time import random blob = Blob( @@ -767,14 +767,16 @@ def get_blob( # Batch.finish() is called, the resulting `NotFound` will be # raised. blob.reload(client=client) - break + except NotFound: + return None except Exception as e: if retry == self.RETRIES - 1: raise e else: rand_num = random.random() - time.sleep(min(rand_num + 2 ** (retry - 1), rand_num + 32)) - return blob + time.sleep(min(rand_num + 2 ** (retry), rand_num + 32)) + else: + return blob def list_blobs( self, diff --git a/storage/tests/unit/test_bucket.py b/storage/tests/unit/test_bucket.py index 530cf9203c53..e57472f59935 100644 --- a/storage/tests/unit/test_bucket.py +++ b/storage/tests/unit/test_bucket.py @@ -676,10 +676,9 @@ def test_get_blob_miss(self): connection = _Connection() client = _Client(connection) bucket = self._make_one(name=NAME) - with self.assertRaises(Exception): - bucket.get_blob(NONESUCH, client=client) + bucket.get_blob(NONESUCH, client=client) num_requests = len(connection._requested) - self.assertGreater(num_requests, 1) + self.assertGreaterEqual(num_requests, 1) kw = connection._requested[-1] self.assertEqual(kw["method"], "GET") self.assertEqual(kw["path"], "/b/%s/o/%s" % (NAME, NONESUCH))