- Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(limit-count): add sliding window support for redis-based policies #12756
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
Open
sihyeonn wants to merge 6 commits into apache:master Choose a base branch from sihyeonn:feat/sh-sliding-window
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits Select commit Hold shift + click to select a range
984e8aa feat: add window_type for limit-count
sihyeonn 9da2fcb feat: add limit-count sliding window for redis with tests
sihyeonn 03e4161 feat: add limit-count sliding window for redis-cluster with tests
sihyeonn 8e6f162 chore: update docs about sliding window
sihyeonn 58c34f1 feat: handling the case of same timestamp
sihyeonn 82ebcb0 chore: add tests for handling same timestamp
sihyeonn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
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.
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.
In the current implementation, each request needs to record an entry in the Redis set. This puts too much pressure on Redis and is probably not a good solution.
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.
@nic-6443
Thanks for raising this concern!
In this sliding-window implementation, we intentionally trade some Redis work per request for exact “N requests per rolling window” semantics. However, the memory and per-key load are bounded:
ZREMRANGEBYSCOREto drop all events outside of the current window and then enforcecurrent + cost <= limitbefore inserting new members,<= limit, and Redis memory for this key is bounded by that limit.Because of this, even though we do one
ZADDper allowed request, we don’t end up with unbounded growth like in some TTL-only designs – the sliding window state per key stays O(limit). Also,window_type = "sliding"is an opt-in mode and the docs explicitly call out that it is more expensive than the default fixed window, so it’s intended for use cases that really need accurate rolling windows and can accept the extra Redis cost.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 suggest you use the approximate sliding window mentioned in this blog https://blog.cloudflare.com/counting-things-a-lot-of-different-things/ to implement this function. You can refer to the code here: https://github.com/ElvinEfendi/lua-resty-global-throttle/blob/main/lib/resty/global_throttle/sliding_window.lua