New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a clocked rate limiter #4591
Conversation
// the original method uses time.Now(), and does not allow us to pass in a time.Time. Fortunately, it can be built on | ||
// top of ReserveN. However, there are some optimizations that we can make. | ||
func (l ClockedRateLimiter) WaitN(ctx context.Context, token int) error { | ||
reservation := ClockedReservation{l.rateLimiter.ReserveN(l.clock.Now(), token), l.clock} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this will do reservation & cancellation more often than the original impl (when token can't be get within the wait duration), so more likely run into golang/go#56924 this issue: keep doing reservation & cancellation result in effectively a lower rps limit (we saw that in prod before)
But I think the change is ok since the only way to avoid ^ issue is to only call Wait() with context that has no deadline, and it still holds for with this change. so I guess this comment is just FYI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh I don't fully understand this issue, but it looks like the Go maintainers closed it because it's working as intended?
2af5b93
to
380aaff
Compare
380aaff
to
7bd2fc3
Compare
What changed?
Our RateLimiterImpl now uses a ClockedRateLimiter instead of a raw go rate limiter which does not accept a clock.
Why?
To make unit testing / time skipping easier in the future, and to add some test coverage.
How did you test it?
I added a bunch of unit tests for the ClockedRateLimiter.
Potential risks
Is hotfix candidate?
No