-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix: Refactors rate limiter within LoggingHandler to be singleton #6693
fix: Refactors rate limiter within LoggingHandler to be singleton #6693
Conversation
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.
LGTM -- nice find and thanks for the fix!
this(ksqlRestConfig, RateLimiter::create); | ||
} | ||
|
||
public LoggingRateLimiter( |
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 looks like it should be package-private and marked as VisibleForTesting?
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.
Done
|
||
public boolean shouldLog(final RoutingContext routingContext) { | ||
if (rateLimitedPaths.containsKey(routingContext.request().path())) { | ||
final String path = routingContext.request().path(); |
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.
Since only the path of the routing context is used here, would it make more sense to pass just the path into this method rather than the entire routing context?
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 debating that a bit. At the moment it only rate limits based upon the path, but in the future, it could be broader. I'm fine to pass path now and revise in the future.
} | ||
|
||
@Test | ||
public void shouldLog_notIncluded() { |
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.
nit (was confused by the name for a while):
public void shouldLog_notIncluded() { | |
public void shouldLog_notRateLimited) { |
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.
Done.
I agree, the name was unclear.
Description
Changes the rate limiting logic within LoggingHandler to be a shared singleton. Otherwise the limit is multiplied by the number of server verticles.
Testing done
Ran unit tests and ran high qps requests manually to verify correct logging.
Reviewer checklist