Skip to content
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

BZ69355: ExactRateLimiter #767

Closed
wants to merge 15 commits into from
Closed

BZ69355: ExactRateLimiter #767

wants to merge 15 commits into from

Conversation

Chenjp
Copy link
Contributor

@Chenjp Chenjp commented Oct 18, 2024

replace #760: redo pr , change src branche from "main" to a dedicated branch "main_BZ69355".
for headers section, compliance with draft spec of "https://datatracker.ietf.org/doc/draft-ietf-httpapi-ratelimit-headers ".
comparing with no-ratelimit / or fast-ratelimiter, ab test of exact-ratelimiter performance shows that no significant throughout decrease (about -2% / -1%).

@Chenjp
Copy link
Contributor Author

Chenjp commented Oct 28, 2024

@markt-asf @rmaucher wondering if this PR is appropriate? tks.

Copy link
Contributor

@markt-asf markt-asf left a comment

Choose a reason for hiding this comment

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

I'm in favour of adding both the headers and the exact rate limiter. The PR just needs some refactoring and clean-up. Generally, several small changes are easier to review than one big change.

public final int getCurrentBucketPrefix() {
return (int) (System.currentTimeMillis() >> this.numBits);
public final int getBucketPrefix(long timestamp) {
return (int) (timestamp >> this.numBits);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make this change? It appears to increase code duplication with no obvious benefit.

java/org/apache/catalina/filters/RateLimitFilter.java Outdated Show resolved Hide resolved
Comment on lines 226 to 241

/**
* Returns the identifier from the servlet request, which used to work as rate limit by.
* @param request
* @return identifier
*/
protected String getLimitByIdentifier(ServletRequest request) {
return request.getRemoteAddr();
}

@Override
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
throws IOException, ServletException {

String ipAddr = request.getRemoteAddr();
int reqCount = rateLimiter.increment(ipAddr);
String identifier = getLimitByIdentifier(request);
int reqCount = rateLimiter.increment(identifier);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like another, possibly separate, improvement.

Comment on lines 55 to 56
timebucket.maintenance.error=定期维护时出现错误
exactRateLimiter.maintenance.error=定期维护时出现错误
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you re-use timebucket.maintenance.error here?

/**
* An accurate fixed window limiter.
*/
public class ExactRateLimiter implements RateLimiter {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there is scope for a common base class for this class and FastRateLimiter

@Chenjp
Copy link
Contributor Author

Chenjp commented Nov 16, 2024

I'm in favour of adding both the headers and the exact rate limiter. The PR just needs some refactoring and clean-up. Generally, several small changes are easier to review than one big change.

@markt-asf smaller PRs came, first one is #775 .

@markt-asf
Copy link
Contributor

No changes to merge here. Looking forward to additional PRs.

@markt-asf markt-asf closed this Nov 28, 2024
@Chenjp
Copy link
Contributor Author

Chenjp commented Nov 28, 2024

another one next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants