From 937be82df689ace3b5038507c93c85390d8352a0 Mon Sep 17 00:00:00 2001 From: Luke Sneeringer Date: Fri, 1 Mar 2019 23:38:46 -0800 Subject: [PATCH] [feat] Retries. (#103) This commit adds automatic retries for both idempotent and non-idempotent requests. --- gapic/schema/wrappers.py | 10 +++ .../$sub/services/$service/client.py.j2 | 19 +++-- tests/system/test_retry.py | 69 +++++++++++++++++++ tests/unit/schema/wrappers/test_method.py | 23 ++++++- 4 files changed, 113 insertions(+), 8 deletions(-) create mode 100644 tests/system/test_retry.py diff --git a/gapic/schema/wrappers.py b/gapic/schema/wrappers.py index 23bd0752c4..692d73d2d8 100644 --- a/gapic/schema/wrappers.py +++ b/gapic/schema/wrappers.py @@ -436,6 +436,16 @@ def grpc_stub_type(self) -> str: server='stream' if self.server_streaming else 'unary', ) + @utils.cached_property + def idempotent(self) -> bool: + """Return True if we know this method is idempotent, False otherwise. + + Note: We are intentionally conservative here. It is far less bad + to falsely believe an idempotent method is non-idempotent than + the converse. + """ + return bool(self.options.Extensions[annotations_pb2.http].get) + @utils.cached_property def ref_types(self) -> Sequence[Union[MessageType, EnumType]]: """Return types referenced by this method.""" diff --git a/gapic/templates/$namespace/$name_$version/$sub/services/$service/client.py.j2 b/gapic/templates/$namespace/$name_$version/$sub/services/$service/client.py.j2 index 05061200f9..3f2b9afe5f 100644 --- a/gapic/templates/$namespace/$name_$version/$sub/services/$service/client.py.j2 +++ b/gapic/templates/$namespace/$name_$version/$sub/services/$service/client.py.j2 @@ -4,8 +4,9 @@ import pkg_resources from typing import Mapping, Optional, Sequence, Tuple, Type, Union +from google.api_core import exceptions from google.api_core import gapic_v1 -from google.api_core import retry +from google.api_core import retry as retries from google.auth import credentials {% for import_ in service.python_modules -%} @@ -54,7 +55,7 @@ class {{ service.name }}: {% for field in method.flattened_fields.values() -%} {{ field.name }}: {{ field.ident }} = None, {% endfor -%} - retry: retry.Retry = None, + retry: retries.Retry = gapic_v1.method.DEFAULT, timeout: float = None, metadata: Sequence[Tuple[str, str]] = (), ) -> {{ method.output.ident }}: @@ -71,7 +72,7 @@ class {{ service.name }}: on the ``request`` instance; if ``request`` is provided, this should not be set. {% endfor -%} - retry (~.retry.Retry): Designation of what errors, if any, + retry (~.retries.Retry): Designation of what errors, if any, should be retried. timeout (float): The timeout for this request. metadata (Sequence[Tuple[str, str]]): Strings which should be @@ -106,8 +107,16 @@ class {{ service.name }}: # and friendly error handling. rpc = gapic_v1.method.wrap_method( self._transport.{{ method.name|snake_case }}, - default_retry=None, # FIXME - default_timeout=None, # FIXME + default_retry=retries.Retry(predicate=retries.if_exception_type( + {%- if method.idempotent %} + exceptions.Aborted, + {%- endif %} + exceptions.ServiceUnavailable, + {%- if method.idempotent %} + exceptions.Unknown, + {%- endif %} + )), + default_timeout=None, client_info=_client_info, ) {%- if method.field_headers %} diff --git a/tests/system/test_retry.py b/tests/system/test_retry.py new file mode 100644 index 0000000000..0cb50eb493 --- /dev/null +++ b/tests/system/test_retry.py @@ -0,0 +1,69 @@ +# Copyright 2019 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import time +from unittest import mock + +import pytest + +from google import showcase_v1alpha3 +from google.api_core import exceptions +from google.rpc import code_pb2 + + +def test_retry_nonidempotent(echo): + # Define our error and OK responses. + err = exceptions.ServiceUnavailable(message='whups') + ok = showcase_v1alpha3.EchoResponse(content='foo') + server = mock.Mock(side_effect=(err, err, ok)) + + # Mock the transport to send back the error responses followed by a + # success response. + transport = type(echo).get_transport_class() + with mock.patch.object(transport, 'echo', + new_callable=mock.PropertyMock(return_value=server)): + with mock.patch.object(time, 'sleep'): + response = echo.echo({'content': 'bar'}) + assert response.content == 'foo' + assert server.call_count == 3 + + +def test_retry_idempotent(identity): + # Define our error and OK responses. + err409 = exceptions.Aborted(message='derp de derp') + err503 = exceptions.ServiceUnavailable(message='whups') + errwtf = exceptions.Unknown(message='huh?') + ok = showcase_v1alpha3.User(name='users/0', display_name='Guido') + server = mock.Mock(side_effect=(err409, err503, errwtf, ok)) + + # Mock the transport to send back the error responses followed by a + # success response. + transport = type(identity).get_transport_class() + with mock.patch.object(transport, 'get_user', + new_callable=mock.PropertyMock(return_value=server)): + with mock.patch.object(time, 'sleep'): + response = identity.get_user({'name': 'users/0'}) + assert response.name == 'users/0' + assert response.display_name == 'Guido' + assert server.call_count == 4 + + +def test_retry_bubble(echo): + with pytest.raises(exceptions.DeadlineExceeded): + echo.echo({ + 'error': { + 'code': code_pb2.Code.Value('DEADLINE_EXCEEDED'), + 'message': 'This took longer than you said it should.', + }, + }) diff --git a/tests/unit/schema/wrappers/test_method.py b/tests/unit/schema/wrappers/test_method.py index 15305ee9c7..6b36ba798f 100644 --- a/tests/unit/schema/wrappers/test_method.py +++ b/tests/unit/schema/wrappers/test_method.py @@ -51,10 +51,27 @@ def test_method_field_headers_none(): assert isinstance(method.field_headers, collections.Sequence) -def test_service_field_headers_present(): +def test_method_field_headers_present(): http_rule = http_pb2.HttpRule(get='/v1/{parent=projects/*}/topics') - service = make_method('DoSomething', http_rule=http_rule) - assert service.field_headers == ('parent',) + method = make_method('DoSomething', http_rule=http_rule) + assert method.field_headers == ('parent',) + + +def test_method_idempotent_yes(): + http_rule = http_pb2.HttpRule(get='/v1/{parent=projects/*}/topics') + method = make_method('DoSomething', http_rule=http_rule) + assert method.idempotent is True + + +def test_method_idempotent_no(): + http_rule = http_pb2.HttpRule(post='/v1/{parent=projects/*}/topics') + method = make_method('DoSomething', http_rule=http_rule) + assert method.idempotent is False + + +def test_method_idempotent_no_http_rule(): + method = make_method('DoSomething') + assert method.idempotent is False def test_method_unary_unary():