-
Notifications
You must be signed in to change notification settings - Fork 671
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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
to
29f178a
Compare
29f178a
to
9af09a2
Compare
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 the https://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. |
Unstale. |
@mitar did you still want to take a look at urllib3's retry class or you want to proceed this way? |
@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? 🙇 |
@mitar Would you mind if someone else took over this pr? Or will you have time to work on this soon? |
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. |
(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.) |
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