Skip to content
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: make backoff_max configurable with default of 120 #2393

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/2393.feature.rst
@@ -0,0 +1,5 @@
Replaces ``Retry.BACK0FF_MAX`` with ``Retry.DEFAULT_BACKOFF_MAX``.

Adds a configurable ``backoff_max`` parameter to the ``Retry`` class.
If a custom ``backoff_max`` is provided to the ``Retry`` class, it
will replace the ``Retry.DEFAULT_BACKOFF_MAX``.
11 changes: 7 additions & 4 deletions src/urllib3/util/retry.py
Expand Up @@ -153,7 +153,7 @@ class Retry:

seconds. If the backoff_factor is 0.1, then :func:`.sleep` will sleep
for [0.0s, 0.2s, 0.4s, ...] between retries. It will never be longer
than :attr:`Retry.BACKOFF_MAX`.
than :attr:`Retry.backoff_max`.

By default, backoff is disabled (set to 0).

Expand Down Expand Up @@ -191,8 +191,8 @@ class Retry:
#: Default headers to be used for ``remove_headers_on_redirect``
DEFAULT_REMOVE_HEADERS_ON_REDIRECT = frozenset(["Authorization"])

#: Maximum backoff time.
BACKOFF_MAX = 120
#: Default maximum backoff time.
DEFAULT_BACKOFF_MAX = 120
Copy link
Member

Choose a reason for hiding this comment

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

This change makes sense, however I think we'd need a deprecation cycle in v1.26.x to remove BACKOFF_MAX safely in v2.0. Perhaps we can do that since it's likely not used often.

In 1.26.x I'm thinking we can go the route of other deprecated Retry classvars: #2000

Copy link
Author

Choose a reason for hiding this comment

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

Ok, great!

To be sure I understand the process, we need a PR to deprecate BACKOFF_MAX. The deprecation change will be included in v1.26.x.

Then this PR (including DEFAULT_BACKOFF_MAX) will be introduced in 2.0.0.

Copy link
Member

Choose a reason for hiding this comment

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

You understand perfectly, once that PR exists and is merged we can merge this PR.


# Backward compatibility; assigned outside of the class.
DEFAULT: ClassVar["Retry"]
Expand All @@ -208,6 +208,7 @@ def __init__(
allowed_methods: Optional[Collection[str]] = DEFAULT_ALLOWED_METHODS,
status_forcelist: Optional[Collection[int]] = None,
backoff_factor: float = 0,
backoff_max: float = DEFAULT_BACKOFF_MAX,
raise_on_redirect: bool = True,
raise_on_status: bool = True,
history: Optional[Tuple[RequestHistory, ...]] = None,
Expand All @@ -230,6 +231,7 @@ def __init__(
self.status_forcelist = status_forcelist or set()
self.allowed_methods = allowed_methods
self.backoff_factor = backoff_factor
self.backoff_max = backoff_max
sethmlarson marked this conversation as resolved.
Show resolved Hide resolved
self.raise_on_redirect = raise_on_redirect
self.raise_on_status = raise_on_status
self.history = history or ()
Expand All @@ -249,6 +251,7 @@ def new(self, **kw: Any) -> "Retry":
allowed_methods=self.allowed_methods,
status_forcelist=self.status_forcelist,
backoff_factor=self.backoff_factor,
backoff_max=self.backoff_max,
raise_on_redirect=self.raise_on_redirect,
raise_on_status=self.raise_on_status,
history=self.history,
Expand Down Expand Up @@ -293,7 +296,7 @@ def get_backoff_time(self) -> float:
return 0

backoff_value = self.backoff_factor * (2 ** (consecutive_errors_len - 1))
return float(min(self.BACKOFF_MAX, backoff_value))
return float(min(self.backoff_max, backoff_value))

def parse_retry_after(self, retry_after: str) -> float:
seconds: float
Expand Down
26 changes: 25 additions & 1 deletion test/test_retry.py
Expand Up @@ -131,7 +131,7 @@ def test_status_counter(self) -> None:

def test_backoff(self) -> None:
""" Backoff is computed correctly """
max_backoff = Retry.BACKOFF_MAX
max_backoff = Retry.DEFAULT_BACKOFF_MAX

retry = Retry(total=100, backoff_factor=0.2)
assert retry.get_backoff_time() == 0 # First request
Expand All @@ -155,6 +155,30 @@ def test_backoff(self) -> None:

assert retry.get_backoff_time() == max_backoff

def test_configurable_backoff_max(self) -> None:
""" Configurable backoff is computed correctly """
max_backoff = 1

retry = Retry(total=100, backoff_factor=0.2, backoff_max=max_backoff)
assert retry.get_backoff_time() == 0 # First request

retry = retry.increment(method="GET")
assert retry.get_backoff_time() == 0 # First retry

retry = retry.increment(method="GET")
assert retry.backoff_factor == 0.2
assert retry.total == 98
assert retry.get_backoff_time() == 0.4 # Start backoff

retry = retry.increment(method="GET")
assert retry.get_backoff_time() == 0.8

retry = retry.increment(method="GET")
assert retry.get_backoff_time() == max_backoff
sethmlarson marked this conversation as resolved.
Show resolved Hide resolved

retry = retry.increment(method="GET")
assert retry.get_backoff_time() == max_backoff

def test_zero_backoff(self) -> None:
retry = Retry()
assert retry.get_backoff_time() == 0
Expand Down