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

feat(remote): retry on TooManyRequests #1636

Closed
wants to merge 1 commit into from
Closed

Conversation

aslafy-z
Copy link
Contributor

@aslafy-z aslafy-z commented Apr 7, 2023

Add HTTP 429 TooManyRequests to the list of retryable status codes.

@imjasonh
Copy link
Collaborator

As previously commented at #1628 (comment)

Retrying in the event of rate limits is a complex subject, and automatically retrying here can make problems worse instead of better.

If we go ahead with #1635 (which mostly seems fine) and you pass in 429 as a retryable code, that seems fine to me, but I wouldn't want to make this the new behavior for every dependency of this code.

But to Jon's comment in #1628 (comment) I wonder if you might be better off just passing in WithTransport and having complete control over the retry behavior yourself, without having to add a new option.

If you've tried that and it was insufficient or painful in some way, that would be helpful for motivating this change.

@aslafy-z
Copy link
Contributor Author

@imjasonh I agree, this change can break things in some setups. I'd prefer to reuse the pre-configured Transport you go-containerregistry provides with all the retry behaviors. Let's close this PR and go ahead with #1635.

@aslafy-z aslafy-z closed this Apr 19, 2023
@aslafy-z aslafy-z deleted the patch-2 branch April 19, 2023 09:29
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.

None yet

2 participants