From a825cdc2392f27c63934bbf05b43ff6ab56453b6 Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Mon, 25 Sep 2017 12:56:51 -0700 Subject: [PATCH 1/7] Add google.api.core.gapic_v1.method --- core/google/api/core/gapic_v1/method.py | 193 ++++++++++++++++++ core/tests/unit/api_core/gapic/test_method.py | 146 +++++++++++++ 2 files changed, 339 insertions(+) create mode 100644 core/google/api/core/gapic_v1/method.py create mode 100644 core/tests/unit/api_core/gapic/test_method.py diff --git a/core/google/api/core/gapic_v1/method.py b/core/google/api/core/gapic_v1/method.py new file mode 100644 index 000000000000..75119c4315c7 --- /dev/null +++ b/core/google/api/core/gapic_v1/method.py @@ -0,0 +1,193 @@ +# Copyright 2017 Google Inc. +# +# 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 +# +# http://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. + +"""Helpers for wrapping low-level gRPC methods with common functionality. + +This is used by gapic clients to provide common error mapping, retry, timeout, +pagination, and long-running operations to gRPC methods. +""" + +import platform + +import pkg_resources + +from google.api.core import retry +from google.api.core import timeout +from google.api.core.helpers import grpc_helpers + +_PY_VERSION = platform.python_version() +_GRPC_VERSION = pkg_resources.get_distribution('grpcio').version +_API_CORE_VERSION = pkg_resources.get_distribution('google-cloud-core').version +METRICS_METADATA_KEY = 'x-goog-api-client' + + +def _is_not_none_or_false(value): + return value is not None and value is not False + + +def _apply_decorators(func, decorators): + """Apply a list of decorators to a given function. + + ``decorators`` may contain items that are ``None`` or ``False`` which will + be ignored. + """ + decorators = list(filter(_is_not_none_or_false, decorators)) + + for decorator in reversed(decorators): + func = decorator(func) + + return func + + +def _prepare_metadata(metadata): + """Transforms metadata to gRPC format and adds global metrics. + + Args: + metadata (Optional[Mapping[str, str]]): Any current metadata. + + Returns: + Sequence[Tuple(str, str)]: The gRPC-friendly metadata keys and values. + """ + if metadata is None: + metadata = {} + + client_metadata = 'api-core/{} gl-python/{} grpc/{}'.format( + _API_CORE_VERSION, _PY_VERSION, _API_CORE_VERSION) + + # Merge this with any existing metric metadata. + client_metadata = ' '.join(filter(None, [ + client_metadata, metadata.get(METRICS_METADATA_KEY, None)])) + + metadata[METRICS_METADATA_KEY] = client_metadata + + return list(metadata.items()) + + +def wrap_method(func, default_retry=None, default_timeout=None, metadata=None): + """Wrap an RPC method with common behavior. + + This applies common error wrapping, retry, and timeout behavior a function. + The wrapped function will take optional ``retry`` and ``timeout`` + arguments. + + For example:: + + import google.api.core.gapic_v1.method + from google.api.core import retry + from google.api.core import timeout + + # The original RPC method. + def get_topic(name, timeout=None): + request = publisher_v2.GetTopicRequest(name=name) + return publisher_stub.GetTopic(request, timeout=timeout) + + default_retry = retry.Retry(deadline=60) + default_timeout = timeout.Timeout(deadline=60) + wrapped_get_topic = google.api.core.gapic_v1.method.wrap_method( + get_topic, default_retry) + + # Execute get_topic with default retry and timeout: + response = wrapped_get_topic() + + # Execute get_topic without doing any retying but with the default + # timeout: + response = wrapped_get_topic(retry=False) + + # Execute get_topic but only retry on 5xx errors: + my_retry = retry.Retry(retry.if_exception_type( + exceptions.InternalServerError)) + response = wrapped_get_topic(retry=my_retry) + + The way this works is by late-wrapping the given function with the retry + and timeout decorators. Essentially, when ``wrapped_get_topic()`` is + called: + + * ``get_topic()`` is first wrapped with the ``timeout`` into + ``get_topic_with_timeout``. + * ``get_topic_with_timeout`` is wrapped with the ``retry`` into + ``get_topic_with_timeout_and_retry()``. + * The final ``get_topic_with_timeout_and_retry`` is called passing through + the ``args`` and ``kwargs``. + + The callstack is therefore:: + + method.__call__() -> + Retry.__call__() -> + Timeout.__call__() -> + wrap_errors() -> + get_topic() + + Note that if ``timeout`` or ``retry`` is ``False``, then they are not + applied to the function. For example, + ``wrapped_get_topic(timeout=False, retry=False)`` is more or less + equivalent to just calling ``get_topic`` but with error re-mapping. + + Args: + func (Callable[Any]): The function to wrap. If timeout is specified, + then this function should take an optional timeout parameter. + default_retry (Optional[google.api.core.Retry]): The default retry + strategy. + default_timeout (Optional[google.api.core.Timeout]): The default + timeout strategy. If an int or float is specified, the timeout will + always be set to that value. + metadata (Union(Mapping[str, str], False)): A dict of metadata keys and + values. This will be augmented with common ``x-google-api-client`` + metadata. If False, metadata will not be passed to the function + at all. + + Returns: + TODO + """ + func = grpc_helpers.wrap_errors(func) + if metadata is not False: + metadata = _prepare_metadata(metadata) + + def method(*args, **kwargs): + """RPC method with retry and timeout.""" + # Note: Due to Python 2 lacking keyword-only arguments we use this + # pattern to extract the retry and timeout params. + retry_ = kwargs.pop('retry', None) + if retry_ is None: + retry_ = default_retry + + timeout_ = kwargs.pop('timeout', None) + if timeout_ is None: + timeout_ = default_timeout + + # If timeout is specified as a number instead of a Timeout instance, + # convert it to a ConstantTimeout. + if timeout_ is not False and isinstance(timeout_, (int, float)): + timeout_ = timeout.ConstantTimeout(timeout_) + + # If timeout is the default and the default timeout is exponential and + # a non-default retry is specified, make sure the timeout's deadline + # matches the retry's. This handles the case where the user leaves + # the timeout default but specifies a lower deadline via the retry. + if (timeout_ is default_timeout + and retry is not False + and retry is not default_retry + and isinstance(timeout_, timeout.ExponentialTimeout)): + timeout_ = timeout_.with_deadline(retry_._deadline) + + # Apply all applicable decorators. + wrapped_func = _apply_decorators(func, [retry_, timeout_]) + + # Set the metadata for the call using the metadata calculated by + # _prepare_metadata. + if metadata is not False: + kwargs['metadata'] = metadata + + return wrapped_func(*args, **kwargs) + + return method diff --git a/core/tests/unit/api_core/gapic/test_method.py b/core/tests/unit/api_core/gapic/test_method.py new file mode 100644 index 000000000000..ef937283d014 --- /dev/null +++ b/core/tests/unit/api_core/gapic/test_method.py @@ -0,0 +1,146 @@ +# Copyright 2017 Google Inc. +# +# 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 +# +# http://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 mock + +from google.api.core import exceptions +from google.api.core import retry +from google.api.core import timeout +import google.api.core.gapic_v1.method + + +def test_wrap_method_basic(): + method = mock.Mock(spec=['__call__'], return_value=42) + + wrapped_method = google.api.core.gapic_v1.method.wrap_method( + method, metadata=False) + + result = wrapped_method(1, 2, meep='moop') + + assert result == 42 + method.assert_called_once_with(1, 2, meep='moop') + + +def test_wrap_method_with_default_metadata(): + method = mock.Mock(spec=['__call__']) + + wrapped_method = google.api.core.gapic_v1.method.wrap_method(method) + + wrapped_method(1, 2, meep='moop') + + method.assert_called_once_with(1, 2, meep='moop', metadata=mock.ANY) + + metadata = method.call_args[1]['metadata'] + assert len(metadata) == 1 + assert metadata[0][0] == 'x-goog-api-client' + assert 'api-core' in metadata[0][1] + + +def test_wrap_method_with_custom_metadata(): + method = mock.Mock(spec=['__call__']) + + wrapped_method = google.api.core.gapic_v1.method.wrap_method( + method, metadata={'foo': 'bar'}) + + wrapped_method(1, 2, meep='moop') + + method.assert_called_once_with(1, 2, meep='moop', metadata=mock.ANY) + + metadata = method.call_args[1]['metadata'] + assert len(metadata) == 2 + assert ('foo', 'bar') in metadata + + +def test_wrap_method_with_merged_metadata(): + method = mock.Mock(spec=['__call__']) + + wrapped_method = google.api.core.gapic_v1.method.wrap_method( + method, metadata={'x-goog-api-client': 'foo/1.2.3'}) + + wrapped_method(1, 2, meep='moop') + + method.assert_called_once_with(1, 2, meep='moop', metadata=mock.ANY) + + metadata = method.call_args[1]['metadata'] + assert len(metadata) == 1 + assert metadata[0][0] == 'x-goog-api-client' + assert metadata[0][1].endswith(' foo/1.2.3') + + +@mock.patch('time.sleep') +def test_wrap_method_with_default_retry_and_timeout(unusued_sleep): + method = mock.Mock(spec=['__call__'], side_effect=[ + exceptions.InternalServerError(None), + 42]) + default_retry = retry.Retry() + default_timeout = timeout.ConstantTimeout(60) + wrapped_method = google.api.core.gapic_v1.method.wrap_method( + method, default_retry, default_timeout) + + result = wrapped_method() + + assert result == 42 + assert method.call_count == 2 + method.assert_called_with(timeout=60, metadata=mock.ANY) + + +@mock.patch('time.sleep') +def test_wrap_method_with_overriding_retry_and_timeout(unusued_sleep): + method = mock.Mock(spec=['__call__'], side_effect=[ + exceptions.NotFound(None), + 42]) + default_retry = retry.Retry() + default_timeout = timeout.ConstantTimeout(60) + wrapped_method = google.api.core.gapic_v1.method.wrap_method( + method, default_retry, default_timeout) + + result = wrapped_method( + retry=retry.Retry(retry.if_exception_type(exceptions.NotFound)), + timeout=timeout.ConstantTimeout(22)) + + assert result == 42 + assert method.call_count == 2 + method.assert_called_with(timeout=22, metadata=mock.ANY) + + +@mock.patch('time.sleep') +def test_wrap_method_with_overriding_retry_deadline(unusued_sleep): + method = mock.Mock(spec=['__call__'], side_effect=([ + exceptions.InternalServerError(None)] * 3) + [42]) + default_retry = retry.Retry() + default_timeout = timeout.ExponentialTimeout(deadline=60) + wrapped_method = google.api.core.gapic_v1.method.wrap_method( + method, default_retry, default_timeout) + + # Overriding only the retry's deadline should also override the timeout's + # deadline. + result = wrapped_method( + retry=default_retry.with_deadline(30)) + + assert result == 42 + timeout_args = [call[1]['timeout'] for call in method.call_args_list] + assert timeout_args == [5, 10, 20, 29] + + +def test_wrap_method_with_overriding_timeout_as_a_number(): + method = mock.Mock(spec=['__call__'], return_value=42) + default_retry = retry.Retry() + default_timeout = timeout.ConstantTimeout(60) + wrapped_method = google.api.core.gapic_v1.method.wrap_method( + method, default_retry, default_timeout) + + result = wrapped_method(timeout=22) + + assert result == 42 + method.assert_called_once_with(timeout=22, metadata=mock.ANY) From 5517874962ae48a5ce682d6b354e079f482fffd2 Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Mon, 25 Sep 2017 13:35:53 -0700 Subject: [PATCH 2/7] Address review comments --- core/google/api/core/gapic_v1/method.py | 115 +++++++++++++++--------- 1 file changed, 72 insertions(+), 43 deletions(-) diff --git a/core/google/api/core/gapic_v1/method.py b/core/google/api/core/gapic_v1/method.py index 75119c4315c7..f0fd212af0fe 100644 --- a/core/google/api/core/gapic_v1/method.py +++ b/core/google/api/core/gapic_v1/method.py @@ -66,14 +66,76 @@ def _prepare_metadata(metadata): _API_CORE_VERSION, _PY_VERSION, _API_CORE_VERSION) # Merge this with any existing metric metadata. - client_metadata = ' '.join(filter(None, [ - client_metadata, metadata.get(METRICS_METADATA_KEY, None)])) + if METRICS_METADATA_KEY in metadata: + client_metadata = '{} {}'.format( + client_metadata, metadata[METRICS_METADATA_KEY]) metadata[METRICS_METADATA_KEY] = client_metadata return list(metadata.items()) +class _GapicCallable(object): + """Callable that applies retry, timeout, and metadata logic.""" + + def __init__(self, target, retry, timeout, metadata): + """ + Args: + target (Callable): The low-level RPC method. If timeout is not + False, should accept a timeout argument. If metadata is not + False, it should accept a metadata argument. + retry (google.api.core.retry.Retry): The default retry for the + callable. If False, this callable will not retry. + timeout (google.api.core.timeout.Timeout): The default timeout + for the callable. If False, this callable will not specify + a timeout argument to the low-level RPC method. + metadata (Sequence[Tuple[str, str]]): gRPC call metadata that's + passed to the low-level RPC method. If False, this callable + will not specify a metadata argument to the method. + """ + self._target = target + self._retry = retry + self._timeout = timeout + self._metadata = metadata + + def __call__(self, *args, **kwargs): + """Invoke the low-level RPC with retry, timeout, and metadata.""" + # Note: Due to Python 2 lacking keyword-only arguments we use this + # pattern to extract the retry and timeout params. + retry_ = kwargs.pop('retry', None) + if retry_ is None: + retry_ = self._retry + + timeout_ = kwargs.pop('timeout', None) + if timeout_ is None: + timeout_ = self._timeout + + # If timeout is specified as a number instead of a Timeout instance, + # convert it to a ConstantTimeout. + if timeout_ is not False and isinstance(timeout_, (int, float)): + timeout_ = timeout.ConstantTimeout(timeout_) + + # If timeout is the default and the default timeout is exponential and + # a non-default retry is specified, make sure the timeout's deadline + # matches the retry's. This handles the case where the user leaves + # the timeout default but specifies a lower deadline via the retry. + if (timeout_ is self._timeout + and retry is not False + and retry is not self._retry + and isinstance(timeout_, timeout.ExponentialTimeout)): + timeout_ = timeout_.with_deadline(retry_._deadline) + + # Apply all applicable decorators. + wrapped_func = _apply_decorators(self._target, [retry_, timeout_]) + + # Set the metadata for the call using the metadata calculated by + # _prepare_metadata. + if self._metadata is not False: + kwargs['metadata'] = self._metadata + + return wrapped_func(*args, **kwargs) + + def wrap_method(func, default_retry=None, default_timeout=None, metadata=None): """Wrap an RPC method with common behavior. @@ -134,8 +196,9 @@ def get_topic(name, timeout=None): equivalent to just calling ``get_topic`` but with error re-mapping. Args: - func (Callable[Any]): The function to wrap. If timeout is specified, - then this function should take an optional timeout parameter. + func (Callable[Any]): The function to wrap. If timeout is not + False, should accept a timeout argument. If metadata is not False, + it should accept a metadata argument. default_retry (Optional[google.api.core.Retry]): The default retry strategy. default_timeout (Optional[google.api.core.Timeout]): The default @@ -147,47 +210,13 @@ def get_topic(name, timeout=None): at all. Returns: - TODO + Callable: A new callable that takes optional ``retry`` and ``timeout`` + arguments and applies the common error mapping, retry, timeout, + and metadata behavior to the low-level RPC method. """ func = grpc_helpers.wrap_errors(func) + if metadata is not False: metadata = _prepare_metadata(metadata) - def method(*args, **kwargs): - """RPC method with retry and timeout.""" - # Note: Due to Python 2 lacking keyword-only arguments we use this - # pattern to extract the retry and timeout params. - retry_ = kwargs.pop('retry', None) - if retry_ is None: - retry_ = default_retry - - timeout_ = kwargs.pop('timeout', None) - if timeout_ is None: - timeout_ = default_timeout - - # If timeout is specified as a number instead of a Timeout instance, - # convert it to a ConstantTimeout. - if timeout_ is not False and isinstance(timeout_, (int, float)): - timeout_ = timeout.ConstantTimeout(timeout_) - - # If timeout is the default and the default timeout is exponential and - # a non-default retry is specified, make sure the timeout's deadline - # matches the retry's. This handles the case where the user leaves - # the timeout default but specifies a lower deadline via the retry. - if (timeout_ is default_timeout - and retry is not False - and retry is not default_retry - and isinstance(timeout_, timeout.ExponentialTimeout)): - timeout_ = timeout_.with_deadline(retry_._deadline) - - # Apply all applicable decorators. - wrapped_func = _apply_decorators(func, [retry_, timeout_]) - - # Set the metadata for the call using the metadata calculated by - # _prepare_metadata. - if metadata is not False: - kwargs['metadata'] = metadata - - return wrapped_func(*args, **kwargs) - - return method + return _GapicCallable(func, default_retry, default_timeout, metadata) From 21fb29f9f37dc41bb2a4d8e6a3f85c13027ed2b3 Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Tue, 26 Sep 2017 09:40:52 -0700 Subject: [PATCH 3/7] Address review comments --- core/google/api/core/gapic_v1/method.py | 39 ++++++++++++------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/core/google/api/core/gapic_v1/method.py b/core/google/api/core/gapic_v1/method.py index f0fd212af0fe..79dc801c63f4 100644 --- a/core/google/api/core/gapic_v1/method.py +++ b/core/google/api/core/gapic_v1/method.py @@ -22,7 +22,6 @@ import pkg_resources -from google.api.core import retry from google.api.core import timeout from google.api.core.helpers import grpc_helpers @@ -42,9 +41,9 @@ def _apply_decorators(func, decorators): ``decorators`` may contain items that are ``None`` or ``False`` which will be ignored. """ - decorators = list(filter(_is_not_none_or_false, decorators)) + decorators = filter(_is_not_none_or_false, reversed(decorators)) - for decorator in reversed(decorators): + for decorator in decorators: func = decorator(func) return func @@ -76,23 +75,23 @@ def _prepare_metadata(metadata): class _GapicCallable(object): - """Callable that applies retry, timeout, and metadata logic.""" + """Callable that applies retry, timeout, and metadata logic. + + Args: + target (Callable): The low-level RPC method. If timeout is not + False, should accept a timeout argument. If metadata is not + False, it should accept a metadata argument. + retry (google.api.core.retry.Retry): The default retry for the + callable. If False, this callable will not retry. + timeout (google.api.core.timeout.Timeout): The default timeout + for the callable. If ``False``, this callable will not specify + a timeout argument to the low-level RPC method. + metadata (Sequence[Tuple[str, str]]): gRPC call metadata that's + passed to the low-level RPC method. If ``False``, this callable + will not specify a metadata argument to the method. + """ def __init__(self, target, retry, timeout, metadata): - """ - Args: - target (Callable): The low-level RPC method. If timeout is not - False, should accept a timeout argument. If metadata is not - False, it should accept a metadata argument. - retry (google.api.core.retry.Retry): The default retry for the - callable. If False, this callable will not retry. - timeout (google.api.core.timeout.Timeout): The default timeout - for the callable. If False, this callable will not specify - a timeout argument to the low-level RPC method. - metadata (Sequence[Tuple[str, str]]): gRPC call metadata that's - passed to the low-level RPC method. If False, this callable - will not specify a metadata argument to the method. - """ self._target = target self._retry = retry self._timeout = timeout @@ -120,8 +119,8 @@ def __call__(self, *args, **kwargs): # matches the retry's. This handles the case where the user leaves # the timeout default but specifies a lower deadline via the retry. if (timeout_ is self._timeout - and retry is not False - and retry is not self._retry + and retry_ is not False + and retry_ is not self._retry and isinstance(timeout_, timeout.ExponentialTimeout)): timeout_ = timeout_.with_deadline(retry_._deadline) From 4f76f9c6fdbfe62752f4f852a2fe4cd54e62799e Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Tue, 26 Sep 2017 10:07:22 -0700 Subject: [PATCH 4/7] Refactor out timeout calculation --- core/google/api/core/gapic_v1/method.py | 87 ++++++++++++++++--------- 1 file changed, 55 insertions(+), 32 deletions(-) diff --git a/core/google/api/core/gapic_v1/method.py b/core/google/api/core/gapic_v1/method.py index 79dc801c63f4..93efd29de91b 100644 --- a/core/google/api/core/gapic_v1/method.py +++ b/core/google/api/core/gapic_v1/method.py @@ -74,6 +74,40 @@ def _prepare_metadata(metadata): return list(metadata.items()) +def _determine_timeout(default_timeout, specified_timeout, retry): + """Determines how timeout should be applied to a wrapped method. + + Args: + default_timeout (Optional[Timeout]): The default timeout specified + at method creation time. + specified_timeout (Optional[Timeout]): The timeout specified at + invocation time. + retry (Optional[Retry]): The retry specified at invocation time. + + Returns: + Optional[Timeout]: The timeout to apply to the method or ``None``. + """ + if specified_timeout is False: + return None + + if specified_timeout is None: + # If timeout is the default and the default timeout is exponential and + # a non-default retry is specified, make sure the timeout's deadline + # matches the retry's. This handles the case where the user leaves + # the timeout default but specifies a lower deadline via the retry. + if retry and isinstance(default_timeout, timeout.ExponentialTimeout): + return default_timeout.with_deadline(retry._deadline) + else: + return default_timeout + + # If timeout is specified as a number instead of a Timeout instance, + # convert it to a ConstantTimeout. + if isinstance(specified_timeout, (int, float)): + return timeout.ConstantTimeout(specified_timeout) + else: + return specified_timeout + + class _GapicCallable(object): """Callable that applies retry, timeout, and metadata logic. @@ -82,9 +116,9 @@ class _GapicCallable(object): False, should accept a timeout argument. If metadata is not False, it should accept a metadata argument. retry (google.api.core.retry.Retry): The default retry for the - callable. If False, this callable will not retry. + callable. If falsy, this callable will not retry. timeout (google.api.core.timeout.Timeout): The default timeout - for the callable. If ``False``, this callable will not specify + for the callable. If falsy, this callable will not specify a timeout argument to the low-level RPC method. metadata (Sequence[Tuple[str, str]]): gRPC call metadata that's passed to the low-level RPC method. If ``False``, this callable @@ -99,33 +133,22 @@ def __init__(self, target, retry, timeout, metadata): def __call__(self, *args, **kwargs): """Invoke the low-level RPC with retry, timeout, and metadata.""" - # Note: Due to Python 2 lacking keyword-only arguments we use this - # pattern to extract the retry and timeout params. - retry_ = kwargs.pop('retry', None) - if retry_ is None: - retry_ = self._retry - - timeout_ = kwargs.pop('timeout', None) - if timeout_ is None: - timeout_ = self._timeout - - # If timeout is specified as a number instead of a Timeout instance, - # convert it to a ConstantTimeout. - if timeout_ is not False and isinstance(timeout_, (int, float)): - timeout_ = timeout.ConstantTimeout(timeout_) - - # If timeout is the default and the default timeout is exponential and - # a non-default retry is specified, make sure the timeout's deadline - # matches the retry's. This handles the case where the user leaves - # the timeout default but specifies a lower deadline via the retry. - if (timeout_ is self._timeout - and retry_ is not False - and retry_ is not self._retry - and isinstance(timeout_, timeout.ExponentialTimeout)): - timeout_ = timeout_.with_deadline(retry_._deadline) + # Note: Due to Python 2 lacking keyword-only arguments we kwargs to + # to extract the retry and timeout params. + timeout_ = _determine_timeout( + self._timeout, + kwargs.pop('timeout', None), + # Use the invocation-specified retry only for this, as we only + # want to adjust the timeout deadline if the *user* specified + # a different retry. + kwargs.get('retry', None)) + + retry = kwargs.pop('retry', None) + if retry is None: + retry = self._retry # Apply all applicable decorators. - wrapped_func = _apply_decorators(self._target, [retry_, timeout_]) + wrapped_func = _apply_decorators(self._target, [retry, timeout_]) # Set the metadata for the call using the metadata calculated by # _prepare_metadata. @@ -195,17 +218,17 @@ def get_topic(name, timeout=None): equivalent to just calling ``get_topic`` but with error re-mapping. Args: - func (Callable[Any]): The function to wrap. If timeout is not - False, should accept a timeout argument. If metadata is not False, - it should accept a metadata argument. + func (Callable[Any]): The function to wrap. It should accept an + optional ``timeout`` argument. If ``metadata`` is not ``False``, it + should accept a ``metadata`` argument. default_retry (Optional[google.api.core.Retry]): The default retry - strategy. + strategy. If falsy, the method will not retry by default. default_timeout (Optional[google.api.core.Timeout]): The default timeout strategy. If an int or float is specified, the timeout will always be set to that value. metadata (Union(Mapping[str, str], False)): A dict of metadata keys and values. This will be augmented with common ``x-google-api-client`` - metadata. If False, metadata will not be passed to the function + metadata. If ``False``, metadata will not be passed to the function at all. Returns: From cdc7f0f2566607381a5109794806f004e458dfca Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Tue, 26 Sep 2017 10:28:43 -0700 Subject: [PATCH 5/7] Moar refactor --- core/google/api/core/gapic_v1/method.py | 54 ++++++++++--------- core/tests/unit/api_core/gapic/test_method.py | 2 +- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/core/google/api/core/gapic_v1/method.py b/core/google/api/core/gapic_v1/method.py index 93efd29de91b..fb6b4ddd89ba 100644 --- a/core/google/api/core/gapic_v1/method.py +++ b/core/google/api/core/gapic_v1/method.py @@ -29,6 +29,7 @@ _GRPC_VERSION = pkg_resources.get_distribution('grpcio').version _API_CORE_VERSION = pkg_resources.get_distribution('google-cloud-core').version METRICS_METADATA_KEY = 'x-goog-api-client' +USE_DEFAULT_METADATA = object() def _is_not_none_or_false(value): @@ -87,10 +88,7 @@ def _determine_timeout(default_timeout, specified_timeout, retry): Returns: Optional[Timeout]: The timeout to apply to the method or ``None``. """ - if specified_timeout is False: - return None - - if specified_timeout is None: + if specified_timeout is default_timeout: # If timeout is the default and the default timeout is exponential and # a non-default retry is specified, make sure the timeout's deadline # matches the retry's. This handles the case where the user leaves @@ -116,13 +114,13 @@ class _GapicCallable(object): False, should accept a timeout argument. If metadata is not False, it should accept a metadata argument. retry (google.api.core.retry.Retry): The default retry for the - callable. If falsy, this callable will not retry. + callable. If ``None``, this callable will not retry by default timeout (google.api.core.timeout.Timeout): The default timeout - for the callable. If falsy, this callable will not specify - a timeout argument to the low-level RPC method. - metadata (Sequence[Tuple[str, str]]): gRPC call metadata that's - passed to the low-level RPC method. If ``False``, this callable - will not specify a metadata argument to the method. + for the callable. If ``None``, this callable will not specify + a timeout argument to the low-level RPC method by default. + metadata (Optional[Sequence[Tuple[str, str]]]): gRPC call metadata + that's passed to the low-level RPC method. If falsy, no metadata + will be passed to the low-level RPC method. """ def __init__(self, target, retry, timeout, metadata): @@ -133,32 +131,32 @@ def __init__(self, target, retry, timeout, metadata): def __call__(self, *args, **kwargs): """Invoke the low-level RPC with retry, timeout, and metadata.""" - # Note: Due to Python 2 lacking keyword-only arguments we kwargs to + # Note: Due to Python 2 lacking keyword-only arguments we use kwargs to # to extract the retry and timeout params. timeout_ = _determine_timeout( self._timeout, - kwargs.pop('timeout', None), - # Use the invocation-specified retry only for this, as we only + kwargs.pop('timeout', self._timeout), + # Use only the invocation-specified retry only for this, as we only # want to adjust the timeout deadline if the *user* specified # a different retry. kwargs.get('retry', None)) - retry = kwargs.pop('retry', None) - if retry is None: - retry = self._retry + retry = kwargs.pop('retry', self._retry) # Apply all applicable decorators. wrapped_func = _apply_decorators(self._target, [retry, timeout_]) # Set the metadata for the call using the metadata calculated by # _prepare_metadata. - if self._metadata is not False: + if self._metadata: kwargs['metadata'] = self._metadata return wrapped_func(*args, **kwargs) -def wrap_method(func, default_retry=None, default_timeout=None, metadata=None): +def wrap_method( + func, default_retry=None, default_timeout=None, + metadata=USE_DEFAULT_METADATA): """Wrap an RPC method with common behavior. This applies common error wrapping, retry, and timeout behavior a function. @@ -186,7 +184,7 @@ def get_topic(name, timeout=None): # Execute get_topic without doing any retying but with the default # timeout: - response = wrapped_get_topic(retry=False) + response = wrapped_get_topic(retry=None) # Execute get_topic but only retry on 5xx errors: my_retry = retry.Retry(retry.if_exception_type( @@ -222,14 +220,15 @@ def get_topic(name, timeout=None): optional ``timeout`` argument. If ``metadata`` is not ``False``, it should accept a ``metadata`` argument. default_retry (Optional[google.api.core.Retry]): The default retry - strategy. If falsy, the method will not retry by default. + strategy. If ``None``, the method will not retry by default. default_timeout (Optional[google.api.core.Timeout]): The default - timeout strategy. If an int or float is specified, the timeout will - always be set to that value. - metadata (Union(Mapping[str, str], False)): A dict of metadata keys and + timeout strategy. Can also be specified as an int or float. If + ``None``, the method will not have timeout specified by default. + metadata (Optional(Mapping[str, str])): A dict of metadata keys and values. This will be augmented with common ``x-google-api-client`` - metadata. If ``False``, metadata will not be passed to the function - at all. + metadata. If ``None``, metadata will not be passed to the function + at all, if ``USE_DEFAULT_METADATA`` (the default) then only the + common metadata will be provided. Returns: Callable: A new callable that takes optional ``retry`` and ``timeout`` @@ -238,7 +237,10 @@ def get_topic(name, timeout=None): """ func = grpc_helpers.wrap_errors(func) - if metadata is not False: + if metadata is USE_DEFAULT_METADATA: + metadata = {} + + if metadata is not None: metadata = _prepare_metadata(metadata) return _GapicCallable(func, default_retry, default_timeout, metadata) diff --git a/core/tests/unit/api_core/gapic/test_method.py b/core/tests/unit/api_core/gapic/test_method.py index ef937283d014..f2e5abb17407 100644 --- a/core/tests/unit/api_core/gapic/test_method.py +++ b/core/tests/unit/api_core/gapic/test_method.py @@ -24,7 +24,7 @@ def test_wrap_method_basic(): method = mock.Mock(spec=['__call__'], return_value=42) wrapped_method = google.api.core.gapic_v1.method.wrap_method( - method, metadata=False) + method, metadata=None) result = wrapped_method(1, 2, meep='moop') From 4b3002a4236c8a1119d206cf67d6bff5154cd9fd Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Tue, 26 Sep 2017 10:32:26 -0700 Subject: [PATCH 6/7] Fix some docstrings --- core/google/api/core/gapic_v1/method.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/core/google/api/core/gapic_v1/method.py b/core/google/api/core/gapic_v1/method.py index fb6b4ddd89ba..3f19145ab58e 100644 --- a/core/google/api/core/gapic_v1/method.py +++ b/core/google/api/core/gapic_v1/method.py @@ -110,9 +110,7 @@ class _GapicCallable(object): """Callable that applies retry, timeout, and metadata logic. Args: - target (Callable): The low-level RPC method. If timeout is not - False, should accept a timeout argument. If metadata is not - False, it should accept a metadata argument. + target (Callable): The low-level RPC method. retry (google.api.core.retry.Retry): The default retry for the callable. If ``None``, this callable will not retry by default timeout (google.api.core.timeout.Timeout): The default timeout @@ -210,14 +208,14 @@ def get_topic(name, timeout=None): wrap_errors() -> get_topic() - Note that if ``timeout`` or ``retry`` is ``False``, then they are not + Note that if ``timeout`` or ``retry`` is ``None``, then they are not applied to the function. For example, - ``wrapped_get_topic(timeout=False, retry=False)`` is more or less + ``wrapped_get_topic(timeout=None, retry=None)`` is more or less equivalent to just calling ``get_topic`` but with error re-mapping. Args: func (Callable[Any]): The function to wrap. It should accept an - optional ``timeout`` argument. If ``metadata`` is not ``False``, it + optional ``timeout`` argument. If ``metadata`` is not ``None``, it should accept a ``metadata`` argument. default_retry (Optional[google.api.core.Retry]): The default retry strategy. If ``None``, the method will not retry by default. From 98ff2a43fb977b583d40e92218cc324e5acafcc6 Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Tue, 26 Sep 2017 10:44:28 -0700 Subject: [PATCH 7/7] Address review comments --- core/google/api/core/gapic_v1/method.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/core/google/api/core/gapic_v1/method.py b/core/google/api/core/gapic_v1/method.py index 3f19145ab58e..8c92200f9252 100644 --- a/core/google/api/core/gapic_v1/method.py +++ b/core/google/api/core/gapic_v1/method.py @@ -54,14 +54,11 @@ def _prepare_metadata(metadata): """Transforms metadata to gRPC format and adds global metrics. Args: - metadata (Optional[Mapping[str, str]]): Any current metadata. + metadata (Mapping[str, str]): Any current metadata. Returns: Sequence[Tuple(str, str)]: The gRPC-friendly metadata keys and values. """ - if metadata is None: - metadata = {} - client_metadata = 'api-core/{} gl-python/{} grpc/{}'.format( _API_CORE_VERSION, _PY_VERSION, _API_CORE_VERSION) @@ -117,7 +114,7 @@ class _GapicCallable(object): for the callable. If ``None``, this callable will not specify a timeout argument to the low-level RPC method by default. metadata (Optional[Sequence[Tuple[str, str]]]): gRPC call metadata - that's passed to the low-level RPC method. If falsy, no metadata + that's passed to the low-level RPC method. If ``None``, no metadata will be passed to the low-level RPC method. """ @@ -130,7 +127,7 @@ def __init__(self, target, retry, timeout, metadata): def __call__(self, *args, **kwargs): """Invoke the low-level RPC with retry, timeout, and metadata.""" # Note: Due to Python 2 lacking keyword-only arguments we use kwargs to - # to extract the retry and timeout params. + # extract the retry and timeout params. timeout_ = _determine_timeout( self._timeout, kwargs.pop('timeout', self._timeout), @@ -146,7 +143,7 @@ def __call__(self, *args, **kwargs): # Set the metadata for the call using the metadata calculated by # _prepare_metadata. - if self._metadata: + if self._metadata is not None: kwargs['metadata'] = self._metadata return wrapped_func(*args, **kwargs) @@ -225,7 +222,7 @@ def get_topic(name, timeout=None): metadata (Optional(Mapping[str, str])): A dict of metadata keys and values. This will be augmented with common ``x-google-api-client`` metadata. If ``None``, metadata will not be passed to the function - at all, if ``USE_DEFAULT_METADATA`` (the default) then only the + at all, if :attr:`USE_DEFAULT_METADATA` (the default) then only the common metadata will be provided. Returns: