Skip to content
Permalink
Browse files
fix: Amend default retry behavior for bucket operations on client (#358)
* fix: Amend default retry behavior for bucket operations on client

* Amend default for blob/bucket get and exist methods

* fix tests for new behavior
  • Loading branch information
andrewsg committed Jan 21, 2021
1 parent 6d5a667 commit b91e57d6ca314ac4feaec30bf355fcf7ac4468c0
Showing with 84 additions and 14 deletions.
  1. +1 −1 google/cloud/storage/blob.py
  2. +2 −2 google/cloud/storage/bucket.py
  3. +3 −4 google/cloud/storage/client.py
  4. +4 −4 tests/unit/test_blob.py
  5. +3 −3 tests/unit/test_bucket.py
  6. +71 −0 tests/unit/test_client.py
@@ -597,7 +597,7 @@ def exists(
if_generation_not_match=None,
if_metageneration_match=None,
if_metageneration_not_match=None,
retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
retry=DEFAULT_RETRY,
):
"""Determines whether or not this blob exists.
@@ -723,7 +723,7 @@ def exists(
timeout=_DEFAULT_TIMEOUT,
if_metageneration_match=None,
if_metageneration_not_match=None,
retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
retry=DEFAULT_RETRY,
):
"""Determines whether or not this bucket exists.
@@ -1108,7 +1108,7 @@ def get_blob(
if_generation_not_match=None,
if_metageneration_match=None,
if_metageneration_not_match=None,
retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
retry=DEFAULT_RETRY,
**kwargs
):
"""Get a blob object by name.
@@ -52,7 +52,6 @@
from google.cloud.storage.acl import DefaultObjectACL
from google.cloud.storage.constants import _DEFAULT_TIMEOUT
from google.cloud.storage.retry import DEFAULT_RETRY
from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED


_marker = object()
@@ -320,7 +319,7 @@ def get_bucket(
timeout=_DEFAULT_TIMEOUT,
if_metageneration_match=None,
if_metageneration_not_match=None,
retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
retry=DEFAULT_RETRY,
):
"""API call: retrieve a bucket via a GET request.
@@ -407,7 +406,7 @@ def lookup_bucket(
timeout=_DEFAULT_TIMEOUT,
if_metageneration_match=None,
if_metageneration_not_match=None,
retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
retry=DEFAULT_RETRY,
):
"""Get a bucket by name, returning None if not found.
@@ -865,7 +864,7 @@ def list_blobs(

path = bucket.path + "/o"
api_request = functools.partial(
self._connection.api_request, timeout=timeout, retry=DEFAULT_RETRY
self._connection.api_request, timeout=timeout, retry=retry
)
iterator = page_iterator.HTTPIterator(
client=self,
@@ -669,7 +669,7 @@ def test_exists_miss(self):
"query_params": {"fields": "name"},
"_target_object": None,
"timeout": 42,
"retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
"retry": DEFAULT_RETRY,
},
)

@@ -692,7 +692,7 @@ def test_exists_hit_w_user_project(self):
"query_params": {"fields": "name", "userProject": USER_PROJECT},
"_target_object": None,
"timeout": self._get_default_timeout(),
"retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
"retry": DEFAULT_RETRY,
},
)

@@ -715,7 +715,7 @@ def test_exists_hit_w_generation(self):
"query_params": {"fields": "name", "generation": GENERATION},
"_target_object": None,
"timeout": self._get_default_timeout(),
"retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
"retry": DEFAULT_RETRY,
},
)

@@ -749,7 +749,7 @@ def test_exists_w_generation_match(self):
},
"_target_object": None,
"timeout": self._get_default_timeout(),
"retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
"retry": DEFAULT_RETRY,
},
)

@@ -672,7 +672,7 @@ def api_request(cls, *args, **kwargs):
"query_params": {"fields": "name"},
"_target_object": None,
"timeout": 42,
"retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
"retry": DEFAULT_RETRY,
}
expected_cw = [((), expected_called_kwargs)]
self.assertEqual(_FakeConnection._called_with, expected_cw)
@@ -707,7 +707,7 @@ def api_request(cls, *args, **kwargs):
},
"_target_object": None,
"timeout": 42,
"retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
"retry": DEFAULT_RETRY,
}
expected_cw = [((), expected_called_kwargs)]
self.assertEqual(_FakeConnection._called_with, expected_cw)
@@ -735,7 +735,7 @@ def api_request(cls, *args, **kwargs):
"query_params": {"fields": "name", "userProject": USER_PROJECT},
"_target_object": None,
"timeout": self._get_default_timeout(),
"retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
"retry": DEFAULT_RETRY,
}
expected_cw = [((), expected_called_kwargs)]
self.assertEqual(_FakeConnection._called_with, expected_cw)
@@ -562,6 +562,54 @@ def test_get_bucket_with_object_hit(self):
parms = dict(urlparse.parse_qsl(qs))
self.assertEqual(parms["projection"], "noAcl")

def test_get_bucket_default_retry(self):
from google.cloud.storage.bucket import Bucket
from google.cloud.storage._http import Connection

PROJECT = "PROJECT"
CREDENTIALS = _make_credentials()
client = self._make_one(project=PROJECT, credentials=CREDENTIALS)

bucket_name = "bucket-name"
bucket_obj = Bucket(client, bucket_name)

with mock.patch.object(Connection, "api_request") as req:
client.get_bucket(bucket_obj)

req.assert_called_once_with(
method="GET",
path=mock.ANY,
query_params=mock.ANY,
headers=mock.ANY,
_target_object=bucket_obj,
timeout=mock.ANY,
retry=DEFAULT_RETRY,
)

def test_get_bucket_respects_retry_override(self):
from google.cloud.storage.bucket import Bucket
from google.cloud.storage._http import Connection

PROJECT = "PROJECT"
CREDENTIALS = _make_credentials()
client = self._make_one(project=PROJECT, credentials=CREDENTIALS)

bucket_name = "bucket-name"
bucket_obj = Bucket(client, bucket_name)

with mock.patch.object(Connection, "api_request") as req:
client.get_bucket(bucket_obj, retry=None)

req.assert_called_once_with(
method="GET",
path=mock.ANY,
query_params=mock.ANY,
headers=mock.ANY,
_target_object=bucket_obj,
timeout=mock.ANY,
retry=None,
)

def test_lookup_bucket_miss(self):
PROJECT = "PROJECT"
CREDENTIALS = _make_credentials()
@@ -658,6 +706,29 @@ def test_lookup_bucket_with_metageneration_match(self):
self.assertEqual(parms["projection"], "noAcl")
self.assertEqual(parms["ifMetagenerationMatch"], str(METAGENERATION_NUMBER))

def test_lookup_bucket_default_retry(self):
from google.cloud.storage.bucket import Bucket
from google.cloud.storage._http import Connection

PROJECT = "PROJECT"
CREDENTIALS = _make_credentials()
client = self._make_one(project=PROJECT, credentials=CREDENTIALS)

bucket_name = "bucket-name"
bucket_obj = Bucket(client, bucket_name)

with mock.patch.object(Connection, "api_request") as req:
client.lookup_bucket(bucket_obj)
req.assert_called_once_with(
method="GET",
path=mock.ANY,
query_params=mock.ANY,
headers=mock.ANY,
_target_object=bucket_obj,
timeout=mock.ANY,
retry=DEFAULT_RETRY,
)

def test_create_bucket_w_missing_client_project(self):
credentials = _make_credentials()
client = self._make_one(project=None, credentials=credentials)

3 comments on commit b91e57d

@AndreaGiardini

This comment has been minimized.

Copy link

@AndreaGiardini AndreaGiardini replied Feb 2, 2021

Hey @andrewsg and thank you for your work :)

we were indeed surprised when we saw

TransportError: ("Failed to retrieve http://metadata.google.internal/computeMetadata/v1/instance/service-accounts/default/?recursive=true from the Google Compute Enginemetadata service. Status: 503 Response:\nb'Service Unavailable\\n'", <google.auth.transport.requests._Response object at 0x7f2983f5d430>)
after running bucket = client.get_bucket(bucket_name) on the latest library version.

Any chance you can release this fix in a new version of the lib?

@andrewsg

This comment has been minimized.

Copy link
Contributor Author

@andrewsg andrewsg replied Feb 2, 2021

Sure thing, building now.

@AndreaGiardini

This comment has been minimized.

Copy link

@AndreaGiardini AndreaGiardini replied Feb 3, 2021

Thank you 👍

Please sign in to comment.