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

enhancement: RateLimitFilter - Provides an exact rate limiting mechanism #794

Closed
wants to merge 6 commits into from

Conversation

Chenjp
Copy link
Contributor

@Chenjp Chenjp commented Dec 6, 2024

smaller PR from #767 .
If you need exact rate limiting and can accept a small decrease in efficiency, ExactRateLimiter may be an alternative option.

you need exact rate limiting and can accept a small decrease in efficiency, ExactRateLimiter may be an alternative option.
@Chenjp Chenjp changed the title enhancemen: RateLimitFilter - Provides an exact rate limiting mechanism enhancement: RateLimitFilter - Provides an exact rate limiting mechanism Dec 6, 2024
@Chenjp
Copy link
Contributor Author

Chenjp commented Dec 16, 2024

minimize API change.

@markt-asf
Copy link
Contributor

You can't remove methods from the RateLimiter interface as it been included in a stable release.

@Chenjp
Copy link
Contributor Author

Chenjp commented Dec 17, 2024

You can't remove methods from the RateLimiter interface as it been included in a stable release.

Updated

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.

The PR makes a very large number of unnecessary changes which makes it extremely hard to review. I have already spend multiple hours just reviewing the refactoring of TimeBucketCounter.

I have yet to decide whether I will proceed with applying those changes as a first step towards to implementing the changes proposed in this PR or whether I'll opt to close this PR and request a series of smaller PRs where each PR addresses a single concern and, potentially, breaks larger, related changes into a series of smaller, reviewable commits.

markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
Based on PR #794 by Chenjp
@markt-asf
Copy link
Contributor

It turns out the TimeBucketCounter was by far the most complex. Once that was reviewed, the rest followed quite quickly. I'm leaving this PR open as there are some changes - particularly the change to eviction - that might need to be added.

markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
Based on PR #794 by Chenjp
markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
Based on PR #794 by Chenjp
markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
Based on PR #794 by Chenjp
@markt-asf
Copy link
Contributor

Closing as I believe all points raised in this PR have now been addressed.

@markt-asf markt-asf closed this Mar 10, 2025
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.

2 participants