Additional throttler types #5
Conversation
* (Re)add hydrator factory * Inject factories as dependancies * Allow throttler specific ttl & limit * Expose throttler ttl & limit
Conflicts: src/RateLimiter.php src/Throttle/Throttler/CacheThrottler.php
Merge upstream master changes
* Corrected config indices for driver creator functions * Added tests for (installed) driver creator functions * Actually use config settings for mysql, memcache
Injected cacheAdapter
Conflicts: README.md config/config.php src/Cache/Factory/DesarrollaCacheFactory.php src/Cache/Factory/FactoryInterface.php src/RateLimiter.php src/Throttle/Hydrator/FactoryInterface.php src/Throttle/Hydrator/HydratorFactory.php tests/Cache/Factory/DesarrollaCacheFactoryTest.php
Conflicts: src/Cache/Factory/DesarrollaCacheFactory.php
* Extracted time based functions * Listener to validate mockery expectations * Added optional threshold
* Throttler settings removed from Data to seperate classes * CacheAdapter dependency moved from Ratelimiter to ThrottlerFactory * Factory can now create LeakyBucketThrottler * Interface Ratelimiter and use settings object * Added various unit tests, moved functional tests
MWW-3452 - Leaky bucket ratelimiter
…es division by 0)
…vision by zero situation when tokenlimit == threshold
Fixes inconsistencies in (milli)seconds in LeakyBucketThrottler.
…n count before delay to allow for calls from multiple sources
…ch actual implementational details
|
@Feijs thank you! |
| public function hit() | ||
| { | ||
| if (0 !== $waitTime = $this->internalThrottler->getRetryTimeout()) { | ||
| $this->timeProvider->usleep(1e3 * $waitTime); |
There was a problem hiding this comment.
May be, better use a constant instead of magic number?
| // Update the window start time if the previous window has passed, or no cached window exists | ||
| try { | ||
| if (($this->timeProvider->now() - $this->cache->get($this->key.self::TIME_CACHE_KEY)) > $this->timeLimit) { | ||
| $this->cache->set($this->key.self::TIME_CACHE_KEY, $this->timeProvider->now(), $this->cacheTtl); |
There was a problem hiding this comment.
i think it's better to use namespaced pools instead of static key suffixes (It may be too much of a think to do btw) :)
There was a problem hiding this comment.
Is there a functional benefit to using namespaces pools? Otherwise that's a bit overkill I think. It'll add the overhead of managing the pools.
| { | ||
| const TIME_CACHE_KEY = ':time'; | ||
| const HITS_CACHE_KEY = ':hits'; | ||
|
|
There was a problem hiding this comment.
Is the better name be like *_SUFFIX
| public function clear() | ||
| { | ||
| $this->setCachedHitCount(0); | ||
| $this->cache->set($this->key.self::TIME_CACHE_KEY, $this->timeProvider->now(), $this->cacheTtl); |
There was a problem hiding this comment.
Isn't it better to use a helper method like 'prefixKey' or 'getTimeCacheKey' since same thing is done multiple times.
|
Thank you @Feijs once again! |
|
Thats wonderfull, thanks! Sorry for the delay in response to your comments, I'll send in another PR soon to address those |
4 participants
Since we needed some other throttling behaviour than offered by the original, I've implemented a number of additional throttlers.
Changes:
Dataobject does no longer need to contain these parameters, so have been removed.CacheAdapteris now a dependency ofThrottlerFactory, instead ofRatelimiterCacheThrottlerhas been renamed toElasticWindowThrottlerto reflect its implementation and be in line with the other throttler names.RatelimiterInterfaceIt does necessarily contain a number of BC breaks. So if you are willing to merge this, I would suggest tagging the current code as a minor release (1.2) and the new code as a new major (2.0)
Sorry for the size of the PR and the interspersed fixes. If preferred I can squash and split the changes into smaller chuncks