Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[RateLimiter] Fix SlidingWindow calculation#51592
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Jeroeny commentedSep 14, 2023
Nyholm left a comment
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.
Thank you for this PR.
This is a bit tricky part of the code base. I suggest some extra eyes to review this.
Can you please add some more test for the newcalculateTimeForToken()?
I also have to ask you to target 6.4 since this is a new feature and not a bugfix. The feature is "Add SlidingWindowLimiter::reserve()".
| return (int)floor($this->hitCountForLastWindow * (1 -$percentOfCurrentTimeFrame) +$this->hitCount); | ||
| } | ||
| publicfunctiongetRetryAfter():\DateTimeImmutable |
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.
You cannot remove public methods. That is a BC break.
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.
My bad, thought the@internal class allowed this.
Jeroeny commentedSep 18, 2023
Reopened as feature PR:#51676 |
…oeny)This PR was squashed before being merged into the 6.4 branch.Discussion----------[RateLimiter] Add SlidingWindowLimiter::reserve()| Q | A| ------------- | ---| Branch? | 6.4| Bug fix? | yes| New feature? | yes| Deprecations? | yes| Tickets |Fix#40289,Fixes#46875| License | MITThis implements `LimiterInterface->reserve()` for `SlidingWindowLimiter`. I'm not sure if the lack of implementation was due to time/scope or if it's actually not possible and my approach is incorrect. But I like to give it a try anyway. Perhaps `@wouterj` you could elaborate on that?The calculation does the following:1. Calculate tokens to be released within this window. E.g. if 4 were used in the last window, at 50% into the current, 2 are still to be released.2. Calculate the time-per-token within the remainder of the window. If the requested tokens will be available before the end of the current window, return the time-per-token * needed-tokens.3. Otherwise return time-per-token of the regular interval * needed-tokens(-after the current window).This wasn't a [bugfix](#51592), so back to feature PR. I couldn't reopen#47557, So had to create this new PR.Commits-------1b4a2df [RateLimiter] Add SlidingWindowLimiter::reserve()
Reopened#47557 but as a bugfix PR targeting 5.4 instead.
This implements
LimiterInterface->reserve()forSlidingWindowLimiter. I'm not sure if the lack of implementation was due to time/scope or if it's actually not possible and my approach is incorrect. But I like to give it a try anyway. Perhaps@wouterj you could elaborate on that?The calculation does the following:
Some other things I've noticed in the RateLimiter:
FixedWindowLimiteruses aWindowclass, whereasSlidingWindowLimiteruses aSlidingWindow. Shouldn't the first have aFixedWindowclass?Windowclass takes its$windowSizeas argument,SlidingWindowdoes not. Perhaps the latter could also take this argument in this PR, since it's used in the calculation. But I guess those classes have to be backwards compatible.SlidingWindow->getRetryAfter()is no longer used and could be deprecated in favor ofcalculateTimeForTokens. Making it more like the fixed window.