Retry retryable 403 (rate limit)#2387
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2387 +/- ##
==========================================
+ Coverage 98.35% 98.36% +0.01%
==========================================
Files 131 132 +1
Lines 13178 13280 +102
==========================================
+ Hits 12961 13063 +102
Misses 217 217
☔ View full report in Codecov by Sentry. |
1588ec3 to
3265cd1
Compare
5225698 to
8b9d62d
Compare
|
I have browsed through this PR and it looks practical to me, but I'm not fully up to speed with the code review requirements of this project to offer an official review. I'm about to pull this branch though and do some testing to see if this works for my use case. Update: |
|
This works great in my testing. Implementation is clean and contained. Very similar to what I have implemented for my internal tools. I have a framework that iterates over many repositories(hundreds) using Happy to help and/or provide any additional feedback where needed. Thanks for the work on this @EnricoMi ! |
|
@dkujawski thanks for testing this! |
|
I can confirm this also works fine in my scripts that work with the entire https://github.com/LineageOS org (around 2500 repositories) |
8b9d62d to
b9575ba
Compare
|
@s-t-e-v-e-n-k @sfdye rebased with latest master, what do you think? |
b9575ba to
16af8e8
Compare
|
This seems so practical - would love to see this land. |
github/GithubRetry.py
Outdated
| ) | ||
|
|
||
| # we backoff primary rate limit at least until X-RateLimit-Reset, | ||
| # we backoff secondary rate limit at for secondaryRateWait seconds |
There was a problem hiding this comment.
In my experience we can use x-ratelimit-reset for secondary wait as well, we don't have to guess
There was a problem hiding this comment.
We could still potentially use secondaryRateWait as the max time we are willing to wait
|
Hello, |
16af8e8 to
eb5aecb
Compare
|
@JLLeitschuh can I get a review here as well, please? |
a1bb64b to
47920ac
Compare
| # used to mock datetime, mock.patch("github.GithubRetry.date") does not work as this | ||
| # references the class, not the module (due to re-exporting in github/__init__.py) | ||
| __datetime = datetime |
There was a problem hiding this comment.
I see you mention mocking here, but the changes don't seem to actually do any mocking. Am I missing something?
There was a problem hiding this comment.
…setters into BasicTestCase" This reverts commit 01ae50a.
47920ac to
9735c1d
Compare
GitHub response with rate limit errors (403 Forbidden) when request occur too frequently. While they can be mitigated by throttling request rate (see #1989), they can still occur.
Not all 403 errors are retry-able (e.g. access forbidden if not authenticated), so retrying based on status code (403) only would be a bad idea. The
urllib3.util.retry.Retryinterface does not allow for checking the response message or body content, only response code: urllib3/urllib3#2500This PR introduces a bespoke PyGithub retry class that is able to determine whether to retry based on the response content. With that, 403 responses that refer to
RateLimitExceededare retried, other 403 responses are not retried. The baseRetryimplementation respects Retry-After headers if they exist.Primary rate limit errors are retried after the primary rate reset occurs, secondary rate limit errors are retried after 60 seconds (configurable).
Log for a primary error rate limit:
Log for a secondary error rate limit: