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

Stop retrying after rate limit errors #146

Merged

Conversation

Mr0grog
Copy link
Member

@Mr0grog Mr0grog commented Dec 4, 2023

We used to retry requests that got a response with a 429 status code (indicating that you've hit a rate limit on the server) automatically, but we've resolved that this is no longer a good approach.

See the discussion in #137 (comment) for more detail, but the short version is that our previous retry behavior was specifically geared to a workflow EDGI used, and which had custom rate limits and other server behavior that most users won't have, and also helped to work around a bunch of deficiencies in our client-side rate limiting that were solved in the last release (but they do need a bit more work in this release, too). It turns out this led to some users getting blocked, and it's better to leave handling this situation to user code.

We used to retry requests that got a response with a 429 status code (indicating that you've hit a rate limit on the server) automatically, but we've resolved that this is no longer a good approach. See the discussion in #137 (comment) for more detail, but the short version is that our previous retry behavior was specifically geared to a workflow EDGI used, and which had custom rate limits and other server behavior that most users won't have, and also helped to work around a bunch of deficiencies in our client-side rate limiting that were solved in the last release (but do need a *bit* more work in this release, too). It turns out this led to some users getting blocked, and it's better to leave handling this situation to user code.
Copy link
Member Author

@Mr0grog Mr0grog left a comment

Choose a reason for hiding this comment

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

I’m holding off for now on re-arranging the actual logic (I just took 429 out of the list of retryable statuses) this I’m mulling through whether we should support someone updating the list of retryable statuses at runtime or subclassing WaybackSession in order to get retries for rate limit errors.

@Mr0grog Mr0grog merged commit 019a0ab into main Dec 6, 2023
@Mr0grog Mr0grog deleted the 137-if-the-server-tells-us-to-stop-we-should-really-just-stop branch December 6, 2023 05:56
@Mr0grog Mr0grog added this to the v0.5.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant