Skip to content

Commit

Permalink
fix: Amend default retry behavior for bucket operations on client (#358)
Browse files Browse the repository at this point in the history 
* 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
andrewsg committed Jan 21, 2021
1 parent 6d5a667 commit b91e57d
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 14 deletions.
2 changes: 1 addition & 1 deletion google/cloud/storage/blob.py
View file
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions google/cloud/storage/bucket.py
View file
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
7 changes: 3 additions & 4 deletions google/cloud/storage/client.py
View file
Expand Up @@ -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()
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
8 changes: 4 additions & 4 deletions tests/unit/test_blob.py
View file
Expand Up @@ -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,
},
)

Expand All @@ -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,
},
)

Expand All @@ -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,
},
)

Expand Down Expand Up @@ -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,
},
)

Expand Down
6 changes: 3 additions & 3 deletions tests/unit/test_bucket.py
View file
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
71 changes: 71 additions & 0 deletions tests/unit/test_client.py
View file
Expand Up @@ -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()
Expand Down Expand Up @@ -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)
Expand Down

3 comments on commit b91e57d

@AndreaGiardini
Copy link

@AndreaGiardini AndreaGiardini commented on b91e57d Feb 2, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing, building now.

@AndreaGiardini
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 👍

Please sign in to comment.