From 4a5b3eeb647486648d99d5d07373747704ee5a09 Mon Sep 17 00:00:00 2001 From: Peter Lamut Date: Thu, 21 Nov 2019 10:20:32 +0100 Subject: [PATCH 1/2] feat(api_core): allow setting Retry deadline as strict If a deadline is set as strict, Retry will shorten the last sleep period to end at the given deadline, and not possibly stretch beyond it. --- api_core/google/api_core/retry.py | 56 +++++++++---- api_core/tests/unit/test_retry.py | 133 ++++++++++++++++++++++++++++-- 2 files changed, 166 insertions(+), 23 deletions(-) diff --git a/api_core/google/api_core/retry.py b/api_core/google/api_core/retry.py index 5962a68b2a87..9cd20566923e 100644 --- a/api_core/google/api_core/retry.py +++ b/api_core/google/api_core/retry.py @@ -141,7 +141,9 @@ def exponential_sleep_generator(initial, maximum, multiplier=_DEFAULT_DELAY_MULT delay = delay * multiplier -def retry_target(target, predicate, sleep_generator, deadline, on_error=None): +def retry_target( + target, predicate, sleep_generator, deadline, on_error=None, strict_deadline=False +): """Call a function and retry if it fails. This is the lowest-level retry helper. Generally, you'll use the @@ -156,9 +158,12 @@ def retry_target(target, predicate, sleep_generator, deadline, on_error=None): sleep_generator (Iterable[float]): An infinite iterator that determines how long to sleep between retries. deadline (float): How long to keep retrying the target. - on_error (Callable): A function to call while processing a retryable - exception. Any error raised by this function will *not* be - caught. + on_error (Callable[Exception]): A function to call while processing a + retryable exception. Any error raised by this function will *not* + be caught. + strict_deadline (bool): If :data:`True`, the last retry will run at + ``deadline``, shortening the last sleep interval as necessary. + Defaults to :data:`False`. Returns: Any: the return value of the target function. @@ -191,16 +196,21 @@ def retry_target(target, predicate, sleep_generator, deadline, on_error=None): on_error(exc) now = datetime_helpers.utcnow() - if deadline_datetime is not None and deadline_datetime < now: - six.raise_from( - exceptions.RetryError( - "Deadline of {:.1f}s exceeded while calling {}".format( - deadline, target + + if deadline_datetime is not None: + if deadline_datetime <= now: + six.raise_from( + exceptions.RetryError( + "Deadline of {:.1f}s exceeded while calling {}".format( + deadline, target + ), + last_exc, ), last_exc, - ), - last_exc, - ) + ) + elif strict_deadline: + time_to_deadline = (deadline_datetime - now).total_seconds() + sleep = min(time_to_deadline, sleep) _LOGGER.debug( "Retrying due to {}, sleeping {:.1f}s ...".format(last_exc, sleep) @@ -228,6 +238,9 @@ class Retry(object): maximum (float): The maximum amout of time to delay in seconds. multiplier (float): The multiplier applied to the delay. deadline (float): How long to keep retrying in seconds. + strict_deadline (bool): If :data:`True`, the last retry will run at + ``deadline``, shortening the last sleep interval as necessary. + Defaults to :data:`False`. """ def __init__( @@ -238,6 +251,7 @@ def __init__( multiplier=_DEFAULT_DELAY_MULTIPLIER, deadline=_DEFAULT_DEADLINE, on_error=None, + strict_deadline=False, ): self._predicate = predicate self._initial = initial @@ -245,14 +259,15 @@ def __init__( self._maximum = maximum self._deadline = deadline self._on_error = on_error + self._strict_deadline = strict_deadline def __call__(self, func, on_error=None): """Wrap a callable with retry behavior. Args: func (Callable): The callable to add retry behavior to. - on_error (Callable): A function to call while processing a - retryable exception. Any error raised by this function will + on_error (Callable[Exception]): A function to call while processing + a retryable exception. Any error raised by this function will *not* be caught. Returns: @@ -275,15 +290,19 @@ def retry_wrapped_func(*args, **kwargs): sleep_generator, self._deadline, on_error=on_error, + strict_deadline=self._strict_deadline ) return retry_wrapped_func - def with_deadline(self, deadline): + def with_deadline(self, deadline, strict_deadline=False): """Return a copy of this retry with the given deadline. Args: deadline (float): How long to keep retrying. + strict_deadline (bool): If :data:`True`, the last retry will run at + ``deadline``, shortening the last sleep interval as necessary. + Defaults to :data:`False`. Returns: Retry: A new retry instance with the given deadline. @@ -295,6 +314,7 @@ def with_deadline(self, deadline): multiplier=self._multiplier, deadline=deadline, on_error=self._on_error, + strict_deadline=strict_deadline, ) def with_predicate(self, predicate): @@ -314,6 +334,7 @@ def with_predicate(self, predicate): multiplier=self._multiplier, deadline=self._deadline, on_error=self._on_error, + strict_deadline=self._strict_deadline, ) def with_delay(self, initial=None, maximum=None, multiplier=None): @@ -335,17 +356,20 @@ def with_delay(self, initial=None, maximum=None, multiplier=None): multiplier=multiplier if maximum is not None else self._multiplier, deadline=self._deadline, on_error=self._on_error, + strict_deadline=self._strict_deadline, ) def __str__(self): return ( "".format( + "multiplier={:.1f}, deadline={:.1f}, on_error={}, " + "strict_deadline={}>".format( self._predicate, self._initial, self._maximum, self._multiplier, self._deadline, self._on_error, + self._strict_deadline, ) ) diff --git a/api_core/tests/unit/test_retry.py b/api_core/tests/unit/test_retry.py index 5b5e59b8eb40..77eb47ce2670 100644 --- a/api_core/tests/unit/test_retry.py +++ b/api_core/tests/unit/test_retry.py @@ -162,6 +162,7 @@ def test_constructor_defaults(self): assert retry_._multiplier == 2 assert retry_._deadline == 120 assert retry_._on_error is None + assert retry_._strict_deadline is False def test_constructor_options(self): _some_function = mock.Mock() @@ -173,6 +174,7 @@ def test_constructor_options(self): multiplier=3, deadline=4, on_error=_some_function, + strict_deadline=True, ) assert retry_._predicate == mock.sentinel.predicate assert retry_._initial == 1 @@ -180,21 +182,62 @@ def test_constructor_options(self): assert retry_._multiplier == 3 assert retry_._deadline == 4 assert retry_._on_error is _some_function + assert retry_._strict_deadline is True def test_with_deadline(self): - retry_ = retry.Retry() - new_retry = retry_.with_deadline(42) + retry_ = retry.Retry( + predicate=mock.sentinel.predicate, + initial=1, + maximum=2, + multiplier=3, + deadline=4, + on_error=mock.sentinel.on_error, + strict_deadline=True, + ) + new_retry = retry_.with_deadline(42, strict_deadline=True) assert retry_ is not new_retry assert new_retry._deadline == 42 + assert new_retry._strict_deadline is True + + # the rest of the attributes should remain the same + assert new_retry._predicate is retry_._predicate + assert new_retry._initial == retry_._initial + assert new_retry._maximum == retry_._maximum + assert new_retry._multiplier == retry_._multiplier + assert new_retry._on_error is retry_._on_error def test_with_predicate(self): - retry_ = retry.Retry() + retry_ = retry.Retry( + predicate=mock.sentinel.predicate, + initial=1, + maximum=2, + multiplier=3, + deadline=4, + on_error=mock.sentinel.on_error, + strict_deadline=True, + ) new_retry = retry_.with_predicate(mock.sentinel.predicate) assert retry_ is not new_retry assert new_retry._predicate == mock.sentinel.predicate + # the rest of the attributes should remain the same + assert new_retry._deadline == retry_._deadline + assert new_retry._strict_deadline == retry_._strict_deadline + assert new_retry._initial == retry_._initial + assert new_retry._maximum == retry_._maximum + assert new_retry._multiplier == retry_._multiplier + assert new_retry._on_error is retry_._on_error + def test_with_delay_noop(self): - retry_ = retry.Retry() + retry_ = retry.Retry( + predicate=mock.sentinel.predicate, + initial=1, + maximum=2, + multiplier=3, + deadline=4, + on_error=mock.sentinel.on_error, + strict_deadline=True, + ) new_retry = retry_.with_delay() assert retry_ is not new_retry assert new_retry._initial == retry_._initial @@ -202,20 +245,47 @@ def test_with_delay_noop(self): assert new_retry._multiplier == retry_._multiplier def test_with_delay(self): - retry_ = retry.Retry() + retry_ = retry.Retry( + predicate=mock.sentinel.predicate, + initial=1, + maximum=2, + multiplier=3, + deadline=4, + on_error=mock.sentinel.on_error, + strict_deadline=True, + ) new_retry = retry_.with_delay(initial=1, maximum=2, multiplier=3) assert retry_ is not new_retry assert new_retry._initial == 1 assert new_retry._maximum == 2 assert new_retry._multiplier == 3 + # the rest of the attributes should remain the same + assert new_retry._deadline == retry_._deadline + assert new_retry._strict_deadline == retry_._strict_deadline + assert new_retry._predicate is retry_._predicate + assert new_retry._on_error is retry_._on_error + def test___str__(self): - retry_ = retry.Retry() + def if_exception_type(exc): + return bool(exc) # pragma: NO COVER + + # Explicitly set all attributes as changed Retry defaults should not + # cause this test to start failing. + retry_ = retry.Retry( + predicate=if_exception_type, + initial=1.0, + maximum=60.0, + multiplier=2.0, + deadline=120.0, + on_error=None, + strict_deadline=False, + ) assert re.match( ( r", " r"initial=1.0, maximum=60.0, multiplier=2.0, deadline=120.0, " - r"on_error=None>" + r"on_error=None, strict_deadline=False>" ), str(retry_), ) @@ -259,6 +329,55 @@ def test___call___and_execute_retry(self, sleep, uniform): sleep.assert_called_once_with(retry_._initial) assert on_error.call_count == 1 + # Make uniform return half of its maximum, which is the calculated sleep time. + @mock.patch("random.uniform", autospec=True, side_effect=lambda m, n: n / 2.0) + @mock.patch("time.sleep", autospec=True) + def test___call___and_execute_retry_strict_deadline(self, sleep, uniform): + + on_error = mock.Mock(spec=["__call__"], side_effect=[None] * 10) + retry_ = retry.Retry( + predicate=retry.if_exception_type(ValueError), + initial=1.0, + maximum=1024.0, + multiplier=2.0, + deadline=9.9, + strict_deadline=True, + ) + + utcnow = datetime.datetime.utcnow() + utcnow_patcher = mock.patch( + "google.api_core.datetime_helpers.utcnow", return_value=utcnow + ) + + target = mock.Mock(spec=["__call__"], side_effect=[ValueError()] * 10) + # __name__ is needed by functools.partial. + target.__name__ = "target" + + decorated = retry_(target, on_error=on_error) + target.assert_not_called() + + with utcnow_patcher as patched_utcnow: + # Make sure that calls to fake time.sleep() also advance the mocked + # time clock. + def increase_time(sleep_delay): + patched_utcnow.return_value += datetime.timedelta(seconds=sleep_delay) + sleep.side_effect = increase_time + + with pytest.raises(exceptions.RetryError): + decorated("meep") + + assert target.call_count == 5 + target.assert_has_calls([mock.call("meep")] * 5) + assert on_error.call_count == 5 + + # check the delays + assert sleep.call_count == 4 # once between each successive target calls + last_wait = sleep.call_args.args[0] + total_wait = sum(call_args.args[0] for call_args in sleep.call_args_list) + + assert last_wait == 2.9 # and not 8.0, because the last delay was shortened + assert total_wait == 9.9 # the same as the (strict) deadline + @mock.patch("time.sleep", autospec=True) def test___init___without_retry_executed(self, sleep): _some_function = mock.Mock() From 9c965ada48c4224ebab84ea9aaebaa2d7dd2146f Mon Sep 17 00:00:00 2001 From: Peter Lamut Date: Mon, 9 Dec 2019 10:53:51 +0100 Subject: [PATCH 2/2] feat(core): make the last retry happen at deadline This commit changes Retry instances in a way that the last sleep period is shortened so that its end matches at the given deadline. This prevents the last sleep period to last way beyond the deadline. --- api_core/google/api_core/retry.py | 35 +++++++++---------------------- api_core/tests/unit/test_retry.py | 20 ++++-------------- 2 files changed, 14 insertions(+), 41 deletions(-) diff --git a/api_core/google/api_core/retry.py b/api_core/google/api_core/retry.py index 9cd20566923e..a1d1f182e8c3 100644 --- a/api_core/google/api_core/retry.py +++ b/api_core/google/api_core/retry.py @@ -141,9 +141,7 @@ def exponential_sleep_generator(initial, maximum, multiplier=_DEFAULT_DELAY_MULT delay = delay * multiplier -def retry_target( - target, predicate, sleep_generator, deadline, on_error=None, strict_deadline=False -): +def retry_target(target, predicate, sleep_generator, deadline, on_error=None): """Call a function and retry if it fails. This is the lowest-level retry helper. Generally, you'll use the @@ -157,13 +155,12 @@ def retry_target( It should return True to retry or False otherwise. sleep_generator (Iterable[float]): An infinite iterator that determines how long to sleep between retries. - deadline (float): How long to keep retrying the target. + deadline (float): How long to keep retrying the target. The last sleep + period is shortened as necessary, so that the last retry runs at + ``deadline`` (and not considerably beyond it). on_error (Callable[Exception]): A function to call while processing a retryable exception. Any error raised by this function will *not* be caught. - strict_deadline (bool): If :data:`True`, the last retry will run at - ``deadline``, shortening the last sleep interval as necessary. - Defaults to :data:`False`. Returns: Any: the return value of the target function. @@ -208,7 +205,7 @@ def retry_target( ), last_exc, ) - elif strict_deadline: + else: time_to_deadline = (deadline_datetime - now).total_seconds() sleep = min(time_to_deadline, sleep) @@ -237,10 +234,9 @@ class Retry(object): must be greater than 0. maximum (float): The maximum amout of time to delay in seconds. multiplier (float): The multiplier applied to the delay. - deadline (float): How long to keep retrying in seconds. - strict_deadline (bool): If :data:`True`, the last retry will run at - ``deadline``, shortening the last sleep interval as necessary. - Defaults to :data:`False`. + deadline (float): How long to keep retrying in seconds. The last sleep + period is shortened as necessary, so that the last retry runs at + ``deadline`` (and not considerably beyond it). """ def __init__( @@ -251,7 +247,6 @@ def __init__( multiplier=_DEFAULT_DELAY_MULTIPLIER, deadline=_DEFAULT_DEADLINE, on_error=None, - strict_deadline=False, ): self._predicate = predicate self._initial = initial @@ -259,7 +254,6 @@ def __init__( self._maximum = maximum self._deadline = deadline self._on_error = on_error - self._strict_deadline = strict_deadline def __call__(self, func, on_error=None): """Wrap a callable with retry behavior. @@ -290,19 +284,15 @@ def retry_wrapped_func(*args, **kwargs): sleep_generator, self._deadline, on_error=on_error, - strict_deadline=self._strict_deadline ) return retry_wrapped_func - def with_deadline(self, deadline, strict_deadline=False): + def with_deadline(self, deadline): """Return a copy of this retry with the given deadline. Args: deadline (float): How long to keep retrying. - strict_deadline (bool): If :data:`True`, the last retry will run at - ``deadline``, shortening the last sleep interval as necessary. - Defaults to :data:`False`. Returns: Retry: A new retry instance with the given deadline. @@ -314,7 +304,6 @@ def with_deadline(self, deadline, strict_deadline=False): multiplier=self._multiplier, deadline=deadline, on_error=self._on_error, - strict_deadline=strict_deadline, ) def with_predicate(self, predicate): @@ -334,7 +323,6 @@ def with_predicate(self, predicate): multiplier=self._multiplier, deadline=self._deadline, on_error=self._on_error, - strict_deadline=self._strict_deadline, ) def with_delay(self, initial=None, maximum=None, multiplier=None): @@ -356,20 +344,17 @@ def with_delay(self, initial=None, maximum=None, multiplier=None): multiplier=multiplier if maximum is not None else self._multiplier, deadline=self._deadline, on_error=self._on_error, - strict_deadline=self._strict_deadline, ) def __str__(self): return ( "".format( + "multiplier={:.1f}, deadline={:.1f}, on_error={}>".format( self._predicate, self._initial, self._maximum, self._multiplier, self._deadline, self._on_error, - self._strict_deadline, ) ) diff --git a/api_core/tests/unit/test_retry.py b/api_core/tests/unit/test_retry.py index 77eb47ce2670..be0c68807518 100644 --- a/api_core/tests/unit/test_retry.py +++ b/api_core/tests/unit/test_retry.py @@ -162,7 +162,6 @@ def test_constructor_defaults(self): assert retry_._multiplier == 2 assert retry_._deadline == 120 assert retry_._on_error is None - assert retry_._strict_deadline is False def test_constructor_options(self): _some_function = mock.Mock() @@ -174,7 +173,6 @@ def test_constructor_options(self): multiplier=3, deadline=4, on_error=_some_function, - strict_deadline=True, ) assert retry_._predicate == mock.sentinel.predicate assert retry_._initial == 1 @@ -182,7 +180,6 @@ def test_constructor_options(self): assert retry_._multiplier == 3 assert retry_._deadline == 4 assert retry_._on_error is _some_function - assert retry_._strict_deadline is True def test_with_deadline(self): retry_ = retry.Retry( @@ -192,12 +189,10 @@ def test_with_deadline(self): multiplier=3, deadline=4, on_error=mock.sentinel.on_error, - strict_deadline=True, ) - new_retry = retry_.with_deadline(42, strict_deadline=True) + new_retry = retry_.with_deadline(42) assert retry_ is not new_retry assert new_retry._deadline == 42 - assert new_retry._strict_deadline is True # the rest of the attributes should remain the same assert new_retry._predicate is retry_._predicate @@ -214,7 +209,6 @@ def test_with_predicate(self): multiplier=3, deadline=4, on_error=mock.sentinel.on_error, - strict_deadline=True, ) new_retry = retry_.with_predicate(mock.sentinel.predicate) assert retry_ is not new_retry @@ -222,7 +216,6 @@ def test_with_predicate(self): # the rest of the attributes should remain the same assert new_retry._deadline == retry_._deadline - assert new_retry._strict_deadline == retry_._strict_deadline assert new_retry._initial == retry_._initial assert new_retry._maximum == retry_._maximum assert new_retry._multiplier == retry_._multiplier @@ -236,7 +229,6 @@ def test_with_delay_noop(self): multiplier=3, deadline=4, on_error=mock.sentinel.on_error, - strict_deadline=True, ) new_retry = retry_.with_delay() assert retry_ is not new_retry @@ -252,7 +244,6 @@ def test_with_delay(self): multiplier=3, deadline=4, on_error=mock.sentinel.on_error, - strict_deadline=True, ) new_retry = retry_.with_delay(initial=1, maximum=2, multiplier=3) assert retry_ is not new_retry @@ -262,7 +253,6 @@ def test_with_delay(self): # the rest of the attributes should remain the same assert new_retry._deadline == retry_._deadline - assert new_retry._strict_deadline == retry_._strict_deadline assert new_retry._predicate is retry_._predicate assert new_retry._on_error is retry_._on_error @@ -279,13 +269,12 @@ def if_exception_type(exc): multiplier=2.0, deadline=120.0, on_error=None, - strict_deadline=False, ) assert re.match( ( r", " r"initial=1.0, maximum=60.0, multiplier=2.0, deadline=120.0, " - r"on_error=None, strict_deadline=False>" + r"on_error=None>" ), str(retry_), ) @@ -332,7 +321,7 @@ def test___call___and_execute_retry(self, sleep, uniform): # Make uniform return half of its maximum, which is the calculated sleep time. @mock.patch("random.uniform", autospec=True, side_effect=lambda m, n: n / 2.0) @mock.patch("time.sleep", autospec=True) - def test___call___and_execute_retry_strict_deadline(self, sleep, uniform): + def test___call___and_execute_retry_hitting_deadline(self, sleep, uniform): on_error = mock.Mock(spec=["__call__"], side_effect=[None] * 10) retry_ = retry.Retry( @@ -341,7 +330,6 @@ def test___call___and_execute_retry_strict_deadline(self, sleep, uniform): maximum=1024.0, multiplier=2.0, deadline=9.9, - strict_deadline=True, ) utcnow = datetime.datetime.utcnow() @@ -376,7 +364,7 @@ def increase_time(sleep_delay): total_wait = sum(call_args.args[0] for call_args in sleep.call_args_list) assert last_wait == 2.9 # and not 8.0, because the last delay was shortened - assert total_wait == 9.9 # the same as the (strict) deadline + assert total_wait == 9.9 # the same as the deadline @mock.patch("time.sleep", autospec=True) def test___init___without_retry_executed(self, sleep):