-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[RateLimiter] Added the docs for the new component #14370
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
Conversation
Basically there are 2 "Sets" of configuration: framework:
rate_limiter:
some_window_limited_limiter:
strategy: fixed_window
interval: '1 min'
limit: 5
some_token_bucket_limiter:
strategy: token_bucket
limit: 5
rate: { interval: '1 min', amount: 1 } In short explanation: A fixed window can be represented by "time windows". They start at the first hit and then allow a maximum number of hits ( A token bucket can be represented as a real bucket. It's initially filled with Just like the docs, the config also needs some iteration I think. I now used the password hashing method of registration, but I'm not too sure. Maybe this is more clean and easier to understand? framework:
rate_limiter:
some_window_limited_limiter:
fixed_window:
interval: 1m
limit: 5
some_token_bucket_limiter:
token_bucket:
limit: 5
rate: { interval: 1m, amount: 1 } |
Thanks for your comments! It's more clear now, so I've updated the docs. About the config ... I prefer the current format instead of the other one proposed below. I still have a question about the |
This is a bit difficult to explain, but it's everything that PHP's
Please note that my previous comment was using a wrong syntax - I've updated it accordingly. |
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.
Sorry for the many comments. There have been 2 major refactorings after the main PR was merged, so quite a few things have changed.
Wouter, thanks for your amazing review 😍 I did all the requested changes! |
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.
Sorry, second round. Thanks for your quick reactions!
This doc PR has revealed quite some good improvements, I'll follow up with a PR on the code repository.
Thanks again for the new review. Thanks to your help this doc is coming along nicely 👍 |
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.
One minor fix left, but other than that ready to be merged imho!
a14d2ef
to
d90991e
Compare
I'd like to merge this soon, so we can later iterate the docs with the changes done in RateLimiter during the stabilization phase before Symfony 5.2 release. Thanks!
Some questions for @wouterj:
strategy
config option mandatory? If yes, could we assign a default value to it to lower the learning curve?interval
option? Is the same as therate.interval
option?