feat: make backoff_max configurable with default of 120 #2393
Conversation
barrett-schonefeld
force-pushed
the
configurable-backoff-max
branch
2 times, most recently
from
37dc408
to
434d06c
Compare
There was a problem hiding this comment.
I think this makes sense to add. Having class-level setting be the only method to configure this isn't great because it impacts all usages of urllib3. The current CI looks like it's failing along with lint, you should run nox -s format to make sure your code is formatted properly and doesn't have type errors.
| #: Maximum backoff time. | ||
| BACKOFF_MAX = 120 | ||
| #: Default maximum backoff time. | ||
| DEFAULT_BACKOFF_MAX = 120 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You understand perfectly, once that PR exists and is merged we can merge this PR.
barrett-schonefeld
force-pushed
the
configurable-backoff-max
branch
from
434d06c
to
191f8da
Compare
Codecov Report
@@ Coverage Diff @@
## main #2393 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 25 25
Lines 2478 2479 +1
=========================================
+ Hits 2478 2479 +1
Continue to review full report at Codecov.
|
|
Hey, folks, checking in on this PR! How might I help get this work ready to be merged? Also, would it be incorrect to leave |
|
@barrett-schonefeld Thanks for the patience here, to answer your questions:
I'm fine with keeping this value as
Unfortunately the 1.x release stream isn't receiving releases including new non-critical features. We're focusing completely on v2.0 so this likely won't be released until then. We don't have a date we're targetting for v2.0 as there still some large TODOs on our plate. Hopefully this doesn't discourage you from contributing this feature. |
There was a problem hiding this comment.
One comment, once we have the PR targeting 1.26.x we can merge this.
In addition we'll want to create a newsfragment, see this directory for examples on how to do so: https://github.com/urllib3/urllib3/tree/main/changelog Let me know if you have questions about this, your fragment should be named 2393.feature.rst and include the change for Retry.BACKOFF_MAX -> Retry.DEFAULT_BACKOFF_MAX and mention the new backoff_max parameter.
barrett-schonefeld
force-pushed
the
configurable-backoff-max
branch
from
191f8da
to
2c156a4
Compare
Updated the test and add to the changelog! |
There was a problem hiding this comment.
Can you please use past tense in the changelog entry as mentioned in https://github.com/urllib3/urllib3/blob/main/changelog/README.rst? (Wasn't clear from the contributing docs, so I updated them: #2415)
As you know, we'll also need the PR targeting the 1.26.x branch deprecating the old attribute first. You can look at #2000 for inspiration.
There was a problem hiding this comment.
After implementing @pquentin's comments (normalize changelog entry, open 1.26.x PR) this PR should be ready to merge.
|
@barrett-schonefeld We're still interested in landing this, but we haven't heard from you in some time, are you still interested in this PR? |
|
Hi @sethmlarson! I am working in the same team as Barrett was working before he left IBM. We are still interested in this feature, so I will probably take over this PR. How should we do this? Since we cannot reach Barrett I think I should create a new branch with his changes and open a new PR? |
|
Right, please open a new PR, we'll close this one. Thanks! |
I work in the IBM Cloud Developer Experience group, and we use this package in our Python SDK "core" library (an enabling layer that is a dependency of our various IBM Cloud Python SDKs).
This message is adapted from an issue we posted in a package we use as a dependency for our Node SDK "core" library. As a reference, the message is adapted from this issue.
We want to use the exponential backoff policy, but also be able to cap the backoff time by some user-configurable amount. It looks like the package supports a default
BACKOFF_TIMEof 120. I would like to utilize thisBACKOFF_TIMEof 120 as the default value and allow users to configure this value as well.This contribution is parallel to the contribution we made to the Node package in this PR.