Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Rate limiter implementation(gcra) #204

Merged
merged 36 commits into from
Jan 17, 2020
Merged

Rate limiter implementation(gcra) #204

merged 36 commits into from
Jan 17, 2020

Conversation

romkatsu
Copy link
Member

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Tests pass? ✔️
Fixed issues #63

previous pull request #203

GCRA

https://brandur.org/rate-limiting
https://blog.ian.stapletoncordas.co/2018/12/understanding-generic-cell-rate-limiting.html

@samdark samdark added the status:code review The pull request needs review. label Jan 10, 2020
@rustamwin
Copy link
Member

It would be great if the logic was ported from Yii2

@samdark
Copy link
Member

samdark commented Jan 11, 2020

@rustamwin GCRA is better about resource consumption than Yii 2 leaky bucket.

@kamarton
Copy link
Contributor

GCRA is good for transactional system, but simple atomic counter design is low cost operation.

  • GCRA require transaction (if need atomcy), GET operation and SET operation (depend on condition) - 2 operations.
  • The atomic counter works with one operation (increment+get), and most cache services support the operation.

src/RateLimiter/CacheCounter.php Outdated Show resolved Hide resolved
src/RateLimiter/CacheCounter.php Outdated Show resolved Hide resolved
src/RateLimiter/RateLimiter.php Outdated Show resolved Hide resolved
src/RateLimiter/RateLimiter.php Outdated Show resolved Hide resolved
src/RateLimiter/RateLimiter.php Outdated Show resolved Hide resolved
src/RateLimiter/CacheCounter.php Outdated Show resolved Hide resolved
src/RateLimiter/CacheCounter.php Outdated Show resolved Hide resolved
src/RateLimiter/CacheCounter.php Outdated Show resolved Hide resolved
src/RateLimiter/CacheCounter.php Outdated Show resolved Hide resolved
@samdark samdark requested a review from a team January 13, 2020 23:37
src/RateLimiter/Counter.php Outdated Show resolved Hide resolved
@samdark samdark closed this Jan 14, 2020
@samdark samdark reopened this Jan 14, 2020
@samdark samdark added the status:ready for merge The pull request is OK to be merged. label Jan 14, 2020
@samdark
Copy link
Member

samdark commented Jan 14, 2020

Another test is planned to ensure that distribution of increments is OK after reaching the limit. @romkatsu is on it.

tests/RateLimiter/CounterTest.php Show resolved Hide resolved
src/RateLimiter/RateLimiterMiddleware.php Outdated Show resolved Hide resolved
src/RateLimiter/CounterStatistics.php Outdated Show resolved Hide resolved
src/RateLimiter/CounterInterface.php Outdated Show resolved Hide resolved
src/RateLimiter/Counter.php Outdated Show resolved Hide resolved
@samdark samdark merged commit 23a4c1b into yiisoft:master Jan 17, 2020
@samdark
Copy link
Member

samdark commented Jan 17, 2020

Thank you very much, @romkatsu for implementing it and everyone else for reviews.

@samdark samdark added type:feature New feature and removed status:code review The pull request needs review. status:ready for merge The pull request is OK to be merged. labels Jan 17, 2020
devanych pushed a commit that referenced this pull request Feb 1, 2021
devanych pushed a commit that referenced this pull request Feb 1, 2021
devanych pushed a commit that referenced this pull request Feb 1, 2021
devanych pushed a commit that referenced this pull request Feb 1, 2021
devanych pushed a commit that referenced this pull request Feb 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type:feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants