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

A little more rate limit tweaking for v0.4.4 #142

Merged
merged 8 commits into from
Nov 28, 2023

Conversation

Mr0grog
Copy link
Member

@Mr0grog Mr0grog commented Nov 27, 2023

This adds a bunch of little tweaks related to the recent rate limit work. It should be more-or-less ready for release after this.

The main thing is adding a 60-second delay for 429 (rate limit) responses. Looking back at the docstring for the RateLimitError class, I think I might have originally intended to always raise instead of retry these, but git history shows it never actually worked that way. I want to get a patch release out with the new retry timings and logs, but patch release doesn’t seem like a good time change whether a response gets retried, so I’ve just made a longer delay here for now.

Then some other assorted bits and bobs:

  • Downgrades the log level when we catch and retry exceptions (used to be WARN, now just INFO). The whole point of retry is to make this situations not special, so I don’t think it’s actually worth a warning.

  • Actually handle all forms of Retry-After header, and move logic to _utils. We didn’t previously handle the date form of the header, just the integer form that indicated a number of seconds. Now we handle both. I also moved the logic out of the exceptions module since it was too much special logic (arguably this was already the case even before I improved the parsing).

  • Fix the total_time calculation for WaybackRetryError. I’m not sure what I was thinking with the previous code, which was… not even sort-of correct.

  • Add read_and_close() everywhere that we raise an exception. We did it in some spots, but had missed others.

This logic has gotten too complex and is probably going to need multiple call-sites, so it doesn't belong directly in the exception constructor anymore.
The 429 status code is a bit special -- the server is warning us we're going to get blocked if we don't slow it down. At current, the Wayback Machine has a 1 minute window for evaluating good behavior, so instead of pausing for the default backoff time, pause for at least 1 minute (if the default backoff time winds up being more, then pause for that).
I'm not sure how we wound up calculating `total_time` as the sum of delays instead of the actual time taken, but we certainly did, and it makes no sense.
@Mr0grog Mr0grog merged commit d3a982e into main Nov 28, 2023
17 checks passed
@Mr0grog Mr0grog deleted the a-little-more-rate-limit-tweaking-for-v0.4.4 branch November 28, 2023 00:09
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