Add configurable option to wait for GitHub external rate limiter#48423
Conversation
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 967b4a1...ea7c94b.
|
…e limiter and monitor
mrnugget
left a comment
There was a problem hiding this comment.
Drive-by comments before hopping into meeting
| // We assert that two requests happened | ||
| assert.True(t, succeeded) | ||
| assert.False(t, hitSecondaryLimit) | ||
| }) |
There was a problem hiding this comment.
I'd add a happy case here too
| } | ||
| var pageOrgs []*github.Org | ||
| pageOrgs, hasNextPage, cost, err = s.v3Client.GetAuthenticatedUserOrgsForPage(ctx, page) | ||
| pageOrgs, hasNextPage, _, err = s.v3Client.GetAuthenticatedUserOrgsForPage(ctx, page) |
There was a problem hiding this comment.
In next iteration, should we remove the cost from returned parameters of that function? Do we still need it in other places?
mrnugget
left a comment
There was a problem hiding this comment.
Cool! Good move on adding that method. I suggest also testing this locally with lots of print-statements to make sure everything really works like it should
kopancek
left a comment
There was a problem hiding this comment.
Looks much cleaner to me, thanks!
|
btw. what about the GitHub v4 client? |
| if remaining := c.retry.Sub(c.now()); remaining > 0 { | ||
| timeutil.SleepWithContext(ctx, remaining) | ||
| return true | ||
| } |
There was a problem hiding this comment.
what the go rate limiter does, and I think we would want here, too is that it also checks the context deadline. If the time by which we could run the request is post-deadline, we can fail immediately instead of sleeping for long.
| for resource, monitor := range map[string]*ratelimit.Monitor{ | ||
| "rest": v3Client.RateLimitMonitor(), | ||
| "rest": v3Client.ExternalRateLimiter(), | ||
| "graphql": v4Client.RateLimitMonitor(), |
There was a problem hiding this comment.
any chance we could get this for v4, too or is this not required? Asking for a batch changes friend.
|
|
||
| GetAuthenticatedOAuthScopes(ctx context.Context) ([]string, error) | ||
| WithAuthenticator(auther auth.Authenticator) client | ||
| SetWaitForRateLimit(wait bool) |
There was a problem hiding this comment.
this seems unused from the diff 🤔
Closes #48540
Sets the default GitHub rate limit to infinite, and adds an option to the V3Client to wait based on the rate limit headers returned by GitHub.
Before this PR
The default rate limit is set to
5000/3600when no rate limit default is set, which severely handicaps our interactions with GitHub. It is also a poor approximation for the actual rate limit from GitHub's side.After this PR
Now, if specified, the client will check the provided headers from GItHub and if a rate limit is hit, it will wait the specified amount of time before retrying. This may be quite a long time depending on which rate limit is hit, but it also does not make sense to fail the job when the rate limit is hit and have impartial data.
Test plan
Tests adjusted.