Skip to content

[RateLimiter] Adding SlidingWindow algorithm #38565

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 15, 2020
Merged

[RateLimiter] Adding SlidingWindow algorithm #38565

merged 1 commit into from
Oct 15, 2020

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Oct 14, 2020

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations?
Tickets
License MIT
Doc PR Should be added

This is a draft PR to make sure we like the idea. I'll keep working on adding tests.

@wouterj
Copy link
Member

wouterj commented Oct 14, 2020

👍 I like this. Thanks!

@wouterj wouterj mentioned this pull request Oct 14, 2020
Merged
@Nyholm Nyholm marked this pull request as ready for review October 14, 2020 11:10
wouterj
wouterj approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Apart from my comments, this looks good to me.

@fabpot would be cool to also have this in 5.2, though it's not 100% needed. Sliding windows generally is better than fixed windows, as they flatten bursts much better.

@nicolas-grekas nicolas-grekas added this to the 5.2 milestone Oct 14, 2020
Nyholm
Nyholm commented Oct 14, 2020
View reviewed changes
src/Symfony/Component/RateLimiter/SlidingWindowLimiter.php
}
}

private function getAvailableTokens(int $hitCount): int
Copy link
Member Author

Choose a reason for hiding this comment

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

This method is public in FixedWindow,

@wouterj is that a bug? It is not part of the interface and never used outside of the class.

OskarStark
OskarStark reviewed Oct 14, 2020
View reviewed changes
fabpot
fabpot reviewed Oct 15, 2020
View reviewed changes
@fabpot
Copy link
Member

fabpot commented Oct 15, 2020

Thank you @Nyholm.

@fabpot fabpot mentioned this pull request Oct 15, 2020
Closed
@fabpot fabpot merged commit 19a3205 into symfony:5.x Oct 15, 2020
@Nyholm
Copy link
Member Author

Nyholm commented Oct 15, 2020

Thank you for merging

@Nyholm Nyholm deleted the sliding-window branch October 15, 2020 07:04
@fabpot fabpot mentioned this pull request Oct 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark left review comments

@fabpot fabpot fabpot approved these changes

@wouterj wouterj wouterj approved these changes

Assignees
No one assigned
Labels
Feature RateLimiter Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
6 participants
@Nyholm @wouterj @fabpot @OskarStark @nicolas-grekas @carsonbot