-
Notifications
You must be signed in to change notification settings - Fork 85
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: adds HTTP retries for responses w/statuses 500-599 #727
Conversation
bd599ad
to
c9da647
Compare
2db96c1
to
c72fb2d
Compare
@BrynCooke / @cecton would y'all mind giving this a once-over? I think I could use some recommendations on my testing strategy/general architecture here. |
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.
Minor nits. Otherwise looks nice!
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.
LGTM~
bbb7be2
to
3c60772
Compare
comments addressed! going to see if this still fails to compile on a few of our targets and will investigate if it does. |
Builds are only failing on our optional nightly runners due to a regression in nightly Rust. I'm not worried about merging this because this is causing a widely used library to fail to compile ( |
fixes #693 - there shouldn't be any visible changes to users, there are no additional messages saying we are retrying, no new error messages saying requests were retried, no new CLI options, this all happens behind the scenes. I've set the max amount of time spent retrying to 10 seconds.