Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Remove rate limiter waiting in perms syncer#47374

Merged
pjlast merged 3 commits into
mainfrom
pjlast/47186-fix-rate-limiting-double-consumption
Feb 3, 2023
Merged

Remove rate limiter waiting in perms syncer#47374
pjlast merged 3 commits into
mainfrom
pjlast/47186-fix-rate-limiting-double-consumption

Conversation

@pjlast

@pjlast pjlast commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

Closes #47186

We already wait for rate limits inside the clients that make requests to the code hosts. Waiting for rate limits in the perms syncer causes double consumption of tokens:

Here you can see the rate limit tokens being reduced twice for a single GitHub request
image

And here it is after removing the wait in the perms syncer:

image

Test plan

Removed unit tests that are no longer necessary. The rest remains the same.

@cla-bot cla-bot Bot added the cla-signed label Feb 3, 2023
@pjlast pjlast force-pushed the pjlast/47186-fix-rate-limiting-double-consumption branch from cfafd76 to db94ac6 Compare February 3, 2023 13:19
@pjlast pjlast requested a review from a team February 3, 2023 13:19
kopancek
kopancek approved these changes Feb 3, 2023
View reviewed changes
Comment thread internal/extsvc/github/common.go Outdated
pjlast and others added 2 commits February 3, 2023 15:23
@pjlast @kopancek
Update internal/extsvc/github/common.go
1770ec4 
Co-authored-by: Milan Freml <kopancek@users.noreply.github.com>
@pjlast
Remove some unused code
e0e6d27
@sourcegraph-bot

sourcegraph-bot commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 35497d5...e0e6d27.

Notify File(s)
@efritz internal/codeintel/dependencies/internal/store/scan.go
@indradhanush enterprise/cmd/repo-updater/internal/authz/metrics.go
enterprise/cmd/repo-updater/internal/authz/perms_syncer.go
enterprise/cmd/repo-updater/internal/authz/perms_syncer_test.go
@mrnugget internal/codeintel/dependencies/internal/store/scan.go
@unknwon enterprise/cmd/repo-updater/internal/authz/metrics.go
enterprise/cmd/repo-updater/internal/authz/perms_syncer.go
enterprise/cmd/repo-updater/internal/authz/perms_syncer_test.go

mrnugget
mrnugget approved these changes Feb 3, 2023
View reviewed changes

@mrnugget mrnugget left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. It was that easy? You made triply sure that we do use a rate limiter in all of the clients we use for auth providers?!

@pjlast

pjlast commented Feb 3, 2023

Copy link
Copy Markdown
Contributor Author

Nice. It was that easy? You made triply sure that we do use a rate limiter in all of the clients we use for auth providers?!

Everything that can get called by fetchUserPermsViaExternalAccounts yes, with the exception of Perforce. I'm still looking at what the implications here are, since Perforce is a bit special

@pjlast

pjlast commented Feb 3, 2023

Copy link
Copy Markdown
Contributor Author

@mrnugget so for Perforce, the perms_syncer makes a request to gitserver, and then gitserver has rate limiting when handling requests: https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph@35497d5527a3eeef9fcd2658dffcd8b17b99a493/-/blob/cmd/gitserver/server/server.go?L347

@pjlast pjlast merged commit 6a5f955 into main Feb 3, 2023
@pjlast pjlast deleted the pjlast/47186-fix-rate-limiting-double-consumption branch February 3, 2023 14:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@mrnugget mrnugget mrnugget approved these changes

@kopancek kopancek kopancek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fix double-consumption of rate limiter tokens when doing permission syncing

4 participants

@pjlast @sourcegraph-bot @mrnugget @kopancek