- Notifications
You must be signed in to change notification settings - Fork675
Also retry HTTP-based transient errors#1648
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-commenter commentedOct 20, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #1648 +/- ##==========================================- Coverage 91.90% 91.80% -0.11%========================================== Files 74 74 Lines 4287 4331 +44 ==========================================+ Hits 3940 3976 +36- Misses 347 355 +8
Flags with carried forward coverage won't be shown.Click here to find out more.
|
2b68c53 to29f178aCompare29f178a to9af09a2Comparenejch commentedNov 2, 2021
Thanks for this@mitar. As this grows (I believe you also added the initial retry flag?), I'm wondering if we should switch to using thehttps://urllib3.readthedocs.io/en/latest/reference/urllib3.util.html#urllib3.util.Retry utility for this. It might reduce our duplication and even clean up our custom 429 handling. Would you like to give it a try here? If not we can do this in a follow-up, but it'd be great if we could at least have tests for this so that we can see it still behaves properly later. One example of similar tests is here, I think: |
This Pull Request (PR) was marked stale because it has been open 90 days with no activity. Please remove the stale label or comment on this PR. Otherwise, it will be closed in 15 days. |
mitar commentedFeb 2, 2022
Unstale. |
nejch commentedFeb 4, 2022
@mitar did you still want to take a look at urllib3's retry class or you want to proceed this way? |
nejch commentedFeb 8, 2022
@mitar as we're getting more requests on this I'm happy to merge this as is and we can work on the rest later, could you just rebase/resolve conflicts please? 🙇 |
Sineaggi commentedFeb 28, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@mitar Would you mind if someone else took over this pr? Or will you have time to work on this soon? |
mitar commentedFeb 28, 2022
Hey, sorry, I got busy with other things. I can add anyone to the branch if you wan to rebase/fix conflicts. I will try to do it as well, but I do not know when I will be able to circle back to this. |
mitar commentedFeb 28, 2022
(But definitely sadly I do not have time to add more tests or to port it to urlib3. I would suggest that this gets merged and then followup work is done to get things further. Also, I have been running this code for some time now and it works great/stable.) |
Uh oh!
There was an error while loading.Please reload this page.
So currently the library can automatically retry when there are some types of API errors. But sometimes the HTTP calls fails on a lower level (connection broken, etc.). I think that should count as a transient error as well and be retried if configured so. This PR adds this.
Closes#1885