Skip to content
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

Merged
merged 4 commits into from
Apr 21, 2024
Merged

Conversation

Doerge
Copy link
Contributor

@Doerge Doerge commented Mar 25, 2024

Adds retries on 500, and a few Hackney specific errors.
Retries are not enabled unless the retry_opts option is given to the request:

AWS.Client.request(..., retry_opts: [max_retries: 3, base_sleep_time: 10, cap_sleep_time: 1000])

I don't fully understand the implications of enabling it by default, hence the caution here.

@onno-vos-dev
Copy link
Member

Closes #120

lib/aws/client.ex Outdated Show resolved Hide resolved
lib/aws/client.ex Outdated Show resolved Hide resolved
@onno-vos-dev
Copy link
Member

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 😄

@Doerge
Copy link
Contributor Author

Doerge commented Mar 27, 2024

I was apparently half asleep, because I had forgotten to actually jitter the sleep time.. 😅
I also added an argument to opts to make it a bit more ergonomic:
AWS.Client.request(..., enable_retries?: true) will now enable it with default values. Only if you want to adjust the specific values are retry_opts needed.
I'd recommend a squash-merge, this ended up a bit messy.

lib/aws/client.ex Outdated Show resolved Hide resolved
Copy link
Member

@onno-vos-dev onno-vos-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

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.
@onno-vos-dev
Copy link
Member

Manually squashed everything down (I don't like doing it from the Github UI). Waiting for the build and will merge then 👍

Copy link
Contributor

@philss philss left a 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 😬

lib/aws/client.ex Show resolved Hide resolved
lib/aws/client.ex Show resolved Hide resolved
lib/aws/client.ex Show resolved Hide resolved
test/aws/client_test.exs Outdated Show resolved Hide resolved
lib/aws/client.ex Outdated Show resolved Hide resolved
lib/aws/client.ex Outdated Show resolved Hide resolved
Copy link
Contributor

@philss philss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet! 🚢

@onno-vos-dev onno-vos-dev self-requested a review April 17, 2024 15:19
@onno-vos-dev
Copy link
Member

I would say let's merge. WDYT @philss ?

@philss
Copy link
Contributor

philss commented Apr 18, 2024

@onno-vos-dev yeah, let's merge! 🚀

@onno-vos-dev onno-vos-dev merged commit a9b4ab8 into aws-beam:master Apr 21, 2024
2 checks passed
@onno-vos-dev
Copy link
Member

Thank you @Doerge will tag a release shortly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants