Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Rate limiter #278
Rate limiter #278
Changes from all commits
d34ac04
48d0f49
1734c78
00d08c1
194d754
a73ff08
6a22321
617e891
303c63c
b43d68b
514a2ab
7ce9662
57576ab
d25f5d7
bc455bb
1652fe2
4be8f12
f00b9fc
a10a18e
c6a3283
9ff2a68
948683b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Curious if we want to apply the rate limiter filter before the
authentication
and afterTracingFilter
. Should we even authenticate the request if there are too many requests?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.
The authentication layer can additionally have its own rate limiting, but it seems reasonable to rate limit authenticated requests like we have here.
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.
Yea I think there's room for both types of limiters. It should be straight forward to extend this work to add a pre-auth limiter, but I'm a bit worried about the scope of this initial PR growing too big. This PR should just aim to be extensible and provide guidance on how one might extend this. I think it does that by creating generic-enough interfaces and providing one full implementation of them.
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.
Sure, can we file an issue to track it?
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.
This seems like it could be a
Semaphore
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.
I think a Semaphore would work as a limit on the number of concurrent requests that blocks instead of rejecting the request. This is a limit on request rate that rejects requests.
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.
Java's Semaphore has a tryAcquire, and you could reject the request if that returns
false
.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.
Ah I see, then it would work if the intent was a concurrent request limit. I think there's room for debate on whether or not there should be a concurrent request limiter but it doesn't feel like that has to be answered in this PR.
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.
why is
tokens
a double?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.
Token acquisition happens during tryAcquire as opposed to happening on a timer, so you acquire partial tokens. Alternatively this could be modeled as
long milliTokens
but I think that would be less legible. I suspect precision isn't an issue because the minimum increment is1/1000
with the latest change to milliseconds.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.
We should check for overflow here too. All of this is kind of scary. I would rather us not write this ourselves if we can use something existing.
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.
+1 to use existing lib if possible. Guava provides an impl. https://guava.dev/releases/23.0/api/docs/com/google/common/util/concurrent/RateLimiter.html.
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.
I think overflowing to infinity is natural/expected if the values are high enough to cause that. I do think we should check for
long
overflows as those wrap around, so I've added some checks there.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.
I was looking at the libraries we already pull in and all of the ones available (including the one you linked above) either don't allow specifying window sizes or are internal APIs that the library doesn't want others to use.
Luckily the amount of code is relatively small and I added some tests.