Revert GitHub refactor (#2379) (including followups) #2669
Conversation
|
@rgmz we've decided to pull the trigger on this. I believe this is a straightforward stack of reverts but please speak up if anything looks amiss. |
| if s.handleRateLimit(err) { | ||
| continue | ||
| break |
There was a problem hiding this comment.
If I'm reading the original refactor commit correctly, this is an appropriate change to revert it. Did I miss some consideration?
There was a problem hiding this comment.
The revert was correct. However, some of these changes are fixes for things that didn't work prior. I'm just pointing out things that need to be re-fixed.
E.g., using break instead of continue means it will never retry these API requests.
| if s.handleRateLimit(err) { | ||
| continue | ||
| break |
| options := &github.ListOptions{ | ||
| PerPage: defaultPagination, | ||
| Page: initialPage, | ||
| } | ||
| for { | ||
| comments, _, err := s.apiClient.Gists.ListComments(ctx, gistID, options) | ||
| if s.handleRateLimit(err) { | ||
| continue | ||
| break |
| if s.handleRateLimit(err) { | ||
| continue | ||
| break |
| if s.handleRateLimit(err) { | ||
| continue | ||
| break |
| @@ -1569,3 +1607,11 @@ func (s *Source) scanTarget(ctx context.Context, target sources.ChunkingTarget, | |||
|
|
|||
| return common.CancellableWrite(ctx, chunksChan, chunk) | |||
| } | |||
|
|
|||
| func removeURLAndSplit(url string) []string { | |||
There was a problem hiding this comment.
I think this would result in a regression compared to getRepoURLParts. e.g., it doesn't work with http:// urls.
There was a problem hiding this comment.
Ah, so the original refactor also fixed a bug? Is that fix something we can independently deploy?
I did a quick scan through and left comments on any obvious issues.
This is surprising to hear. I'm not aware of any interaction that would result in more traffic over a smaller period of time, but I'm also not aware of all the use cases. |
The refactor added a new API call that's invoked for each repository enumerated. When many repositories are enumerated, this results in show-stopping rate limiting. |
Ah, I see. That was unintentional and should have been fixed in #2625. It seems like that didn't land until the most recent release (v3.72.0), though. The change definitely could have used another pass with a fresh pair of eyes before being merged. I think part of the issue was that I hadn't touched it for quite a while prior to it being merged. |
|
Ah, got it. With that pointer we dug a little further and it appears that we were just bitten by some very unfortunate release timing - the rate limit issue has, in fact, been resolved. Thanks for your patience and your eyes on this @rgmz! |
Description:
The GitHub source refactor in #2379 reduced the overall number of requests to GitHub, but compressed them into a smaller period of time, resulting in more rate limit pressure that is causing noticeable degradation in usability in some cases. We've decided to back it out until we can figure out how to make improvements without this interim usability degradation. That PR had multiple followup PRs (#2608, #2614, #2625), which this PR also reverts.
Note that this doesn't revert the
go-githubversion bump from 57 to 61 that has since occurred.Checklist:
make test-community)?make lintthis requires golangci-lint)?