Skip to content

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.

@aslafy-z aslafy-z marked this pull request as ready for review April 7, 2023 13:20
@imjasonh
Copy link
Contributor

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.

2 participants