Retry requests if GitLab external rate limit is hit#48616
Conversation
5c47e96 to
30c9851
Compare
b7851fe to
8cf5b92
Compare
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 62a93a2...d09e3be.
|
sashaostrikov
left a comment
There was a problem hiding this comment.
LGTM!
I've added a comment about a test for possible forgotten mutex unlock.
There was a problem hiding this comment.
nit: ignore explicitly?
| c.externalRateLimiter.WaitForRateLimit(ctx) | |
| _ = c.externalRateLimiter.WaitForRateLimit(ctx) |
vdavid
left a comment
There was a problem hiding this comment.
Wow, very nice! I thought it'd be a lot of time before we see this work for GitLab, too!
7f2af46 to
782b7e0
Compare
There was a problem hiding this comment.
GitLab.com has a lot of different rate limits, depending on which API you use: https://docs.gitlab.com/ee/user/gitlab_com/index.html#gitlabcom-specific-rate-limits
Are we sure that all of the uses of this method here use the same API?
I'm scared that if we use doWithBaseURL for different APIs with different rate limits we'd flip-flop between rate limits depending on the last request.
There was a problem hiding this comment.
From what I understand, GitLab only responds with rate limit headers if the rate limit has been hit. So the only time in which a rate limit should be relevant is if we're immediately going to retry the request. I also think we only really use the Authenticated API endpoints
507d35d to
535ec72
Compare
Closes #48614
Before this PR
GitLab requests all share a single global internal rate limiter. This is a very crude and inaccurate approximation of GitLab's actual rate limits.
After this PR
GitLab requests now use GitLab's response headers to keep track of individual user rate limits. This allows Sourcegraph to make a lot more requests and stay up to date faster while staying inside GitLab's rate limits.
Test plan
Unit tests added.