-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: use custom exponential sleep generator #860
Changes from all commits
aa1e40e
b059139
3ed2168
5199839
4e14bf3
cbb95c9
f43aac1
91fc1e6
06e5276
62b8e48
237e051
868ff2e
21a5077
02f0c09
456caba
f3627c1
bcc02d7
5e7c156
604d3d8
14f359d
858c57d
36a3153
07b39b1
5d90478
b69da5a
df3ea47
8dcd444
94a8684
72b8d1b
8496211
71ba0ea
94e98db
320d157
5064870
b0aafec
23aa2fd
375afaf
72ecf96
187eeba
85445f0
1995a1f
4eab6a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
|
||
from typing import Callable, Any | ||
import time | ||
import random | ||
|
||
from google.api_core import exceptions as core_exceptions | ||
from google.cloud.bigtable.data.exceptions import RetryExceptionGroup | ||
|
@@ -62,6 +63,51 @@ def _attempt_timeout_generator( | |
yield max(0, min(per_request_timeout, deadline - time.monotonic())) | ||
|
||
|
||
def _exponential_sleep_generator( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the min increase is 0 and the multiplier is 1 then this would loop forever right? Maybe an error message for this case might be useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a generator function, so there is no risk of an infinite loop. The method "returns" at the yield line, only to pick up there again the next time If the min increase is 0 and the multiplier is 1, it will continuously spit out the |
||
initial: float = 0.01, | ||
maximum: float = 60, | ||
multiplier: float = 2, | ||
min_increase: float = 0.01, | ||
): | ||
""" | ||
Generates sleep intervals for exponential backoff on failed rpc attempts. | ||
|
||
Based on google.api_core.retry.exponential_sleep_generator, | ||
but with the added constraint that each sleep interval must be strictly | ||
greater than the previous one. | ||
|
||
Args: | ||
initial: The starting delay value, in seconds. Subsequent values will | ||
always be less than this value. Must be > 0. | ||
maximum: The maximum amount of time to delay, in seconds. Must be | ||
>= initial. | ||
multiplier: The multiplier applied to the delay. Modifies the upper range | ||
of sleep values that may be returned. Must be >= 1. | ||
min_increase: The minimum amount of time to increase the delay, | ||
in seconds. Modifies the lower range of sleep values that may be | ||
returned. Min_increase will not be applied if it would cause the | ||
value to exceed maximum. Must be >= 0. | ||
Yields: | ||
float: successive sleep intervals for exponential backoff, in seconds. | ||
""" | ||
if initial <= 0: | ||
raise ValueError("initial must be > 0") | ||
if multiplier < 1: | ||
raise ValueError("multiplier must be >= 1") | ||
if maximum < initial: | ||
raise ValueError("maximum must be >= initial") | ||
if min_increase < 0: | ||
raise ValueError("min_increase must be >= 0") | ||
lower_bound = initial | ||
upper_bound = initial | ||
next_sleep = initial | ||
while True: | ||
yield next_sleep | ||
lower_bound = min(next_sleep + min_increase, maximum) | ||
upper_bound = min(max(upper_bound * multiplier, lower_bound), maximum) | ||
next_sleep = random.uniform(lower_bound, upper_bound) | ||
|
||
|
||
# TODO:replace this function with an exception_factory passed into the retry when | ||
# feature is merged: | ||
# https://github.com/googleapis/python-bigtable/blob/ea5b4f923e42516729c57113ddbe28096841b952/google/cloud/bigtable/data/_async/_read_rows.py#L130 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,6 +97,112 @@ def test_attempt_timeout_w_sleeps(self): | |
expected_value -= sleep_time | ||
|
||
|
||
class TestExponentialSleepGenerator: | ||
@mock.patch("random.uniform", autospec=True, side_effect=lambda m, n: m) | ||
@pytest.mark.parametrize( | ||
"args,expected", | ||
[ | ||
((), [0.01, 0.02, 0.03, 0.04, 0.05]), # test defaults | ||
((1, 3, 2, 1), [1, 2, 3, 3, 3]), # test hitting limit | ||
((1, 3, 2, 0.5), [1, 1.5, 2, 2.5, 3, 3]), # test with smaller min_increase | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't the tests seem to be using the multiplier? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the next_sleep value is calculated using |
||
((0.92, 3, 2, 0), [0.92, 0.92, 0.92]), # test with min_increase of 0 | ||
((1, 3, 10, 0.5), [1, 1.5, 2, 2.5, 3, 3]), # test with larger multiplier | ||
((1, 25, 1.5, 5), [1, 6, 11, 16, 21, 25]), # test with larger min increase | ||
((1, 5, 1, 0), [1, 1, 1, 1]), # test with multiplier of 1 | ||
((1, 5, 1, 1), [1, 2, 3, 4]), # test with min_increase of 1 and multiplier of 1 | ||
], | ||
) | ||
def test_exponential_sleep_generator_lower_bound(self, uniform, args, expected): | ||
""" | ||
Test that _exponential_sleep_generator generated expected values when random.uniform is mocked to return | ||
the lower bound of the range | ||
|
||
Each yield should consistently be min_increase above the last | ||
""" | ||
import itertools | ||
|
||
gen = _helpers._exponential_sleep_generator(*args) | ||
result = list(itertools.islice(gen, len(expected))) | ||
assert result == expected | ||
|
||
@mock.patch("random.uniform", autospec=True, side_effect=lambda m, n: n) | ||
@pytest.mark.parametrize( | ||
"args,expected", | ||
[ | ||
((), [0.01, 0.02, 0.04, 0.08, 0.16, 0.32, 0.64, 1.28]), # test defaults | ||
((1, 3, 2, 1), [1, 2, 3, 3, 3]), # test hitting limit | ||
((1, 3, 2, 0.5), [1, 2, 3, 3]), # test with smaller min_increase | ||
((0.92, 3, 2, 0), [0.92, 1.84, 3, 3]), # test with min_increase of 0 | ||
((1, 5000, 10, 0.5), [1, 10, 100, 1000]), # test with larger multiplier | ||
((1, 20, 1.5, 5), [1, 6, 11, 16.5, 20]), # test with larger min increase | ||
((1, 5, 1, 0), [1, 1, 1, 1]), # test with multiplier of 1 | ||
((1, 5, 1, 1), [1, 2, 3, 4]), # test with min_increase of 1 and multiplier of 1 | ||
], | ||
) | ||
def test_exponential_sleep_generator_upper_bound(self, uniform, args, expected): | ||
""" | ||
Test that _exponential_sleep_generator generated expected values when random.uniform is mocked to return | ||
the upper bound of the range | ||
|
||
Each yield should be scaled by multiplier | ||
""" | ||
import itertools | ||
|
||
gen = _helpers._exponential_sleep_generator(*args) | ||
result = list(itertools.islice(gen, len(expected))) | ||
assert result == expected | ||
|
||
@pytest.mark.parametrize( | ||
"kwargs,exc_msg", | ||
[ | ||
({"initial": 0}, "initial must be > 0"), | ||
({"initial": -1}, "initial must be > 0"), | ||
({"multiplier": 0}, "multiplier must be >= 1"), | ||
({"multiplier": -1}, "multiplier must be >= 1"), | ||
({"multiplier": 0.9}, "multiplier must be >= 1"), | ||
({"min_increase": -1}, "min_increase must be >= 0"), | ||
({"min_increase": -0.1}, "min_increase must be >= 0"), | ||
({"initial": 1, "maximum": 0}, "maximum must be >= initial"), | ||
({"initial": 2, "maximum": 1}, "maximum must be >= initial"), | ||
({"initial": 2, "maximum": 1.99}, "maximum must be >= initial"), | ||
], | ||
) | ||
def test_exponential_sleep_generator_bad_arguments(self, kwargs, exc_msg): | ||
""" | ||
Test that _exponential_sleep_generator raises ValueError when given unexpected 0 or negative values | ||
""" | ||
with pytest.raises(ValueError) as excinfo: | ||
gen = _helpers._exponential_sleep_generator(**kwargs) | ||
# start generator | ||
next(gen) | ||
assert exc_msg in str(excinfo.value) | ||
|
||
@pytest.mark.parametrize( | ||
"kwargs", | ||
[ | ||
{}, | ||
{"multiplier": 1}, | ||
{"multiplier": 1.1}, | ||
{"multiplier": 2}, | ||
{"min_increase": 0}, | ||
{"min_increase": 0.1}, | ||
{"min_increase": 100}, | ||
{"multiplier": 1, "min_increase": 0}, | ||
{"multiplier": 1, "min_increase": 4}, | ||
], | ||
) | ||
def test_exponential_sleep_generator_always_increases(self, kwargs): | ||
""" | ||
Generate a bunch of sleep values without random mocked, to ensure they always increase | ||
""" | ||
gen = _helpers._exponential_sleep_generator(**kwargs, maximum=float("inf")) | ||
last = next(gen) | ||
for i in range(100): | ||
current = next(gen) | ||
assert current >= last | ||
last = current | ||
|
||
|
||
class TestConvertRetryDeadline: | ||
""" | ||
Test _convert_retry_deadline wrapper | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been looking at https://www.learnpython.org/en/Partial_functions to try to understand this a little bit better. I guess we are expecting this
retries.retry_target
to be in place of theretry
function defined before?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this will be a little complicated without context.
AsyncRetry is just a wrapper around retries.retry_target. But AsynRetry doesn't expose the sleep_generator argument we need to customize here. So I'm replacing the AsyncRetry instance with the lower-level retries.retry_target.
The partial allows us to prepare the function's arguments without calling it yet. In the future, we can call
retry_wrapped
without any arguments, and it will call it with everything we passed into the partial here. This is useful for things like retries, where we may not always have all the context to re-build the entire operation when we want to launch a new attempt