-
Notifications
You must be signed in to change notification settings - Fork 132
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
Automatic retries #182
Automatic retries #182
Conversation
Closes #120 |
Thank you @Doerge for picking up this open issue! 🎉 ❤️ I've left two comments, one I believe is a must to maintain backwards compatibility and not retry unless specifically asked for. Unless I'm mistaken 😄 |
I was apparently half asleep, because I had forgotten to actually jitter the sleep time.. 😅 |
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.
Will wait a day or two to see if @philss finds anything I missed but otherwise I'll merge this on Friday when I have some time set aside for aws-beam/aws-codegen#109 and I can deal with both at once as well as roll out a new release 🥳🥳🥳
- Retrying is turned off by default. - Hackney specific errors are retried too. - Add retry test, and format.
7137a83
to
f4783ed
Compare
Manually squashed everything down (I don't like doing it from the Github UI). Waiting for the build and will merge then 👍 |
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.
Hey, it looks good to me!
I added a few minor suggestions.
If it's too much for this PR, we can tackle that in another one.
And sorry for the delay to review 😬
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.
Sweet! 🚢
I would say let's merge. WDYT @philss ? |
@onno-vos-dev yeah, let's merge! 🚀 |
Thank you @Doerge will tag a release shortly |
Adds retries on 500, and a few Hackney specific errors.
Retries are not enabled unless the
retry_opts
option is given to the request:I don't fully understand the implications of enabling it by default, hence the caution here.