Refactor rate limiter#1118
Conversation
* Make RateLimiter support burst * Make DynamicRateLimiter handles periodical refresh of rate / burst
alexshtin
left a comment
alexshtin
left a comment
There was a problem hiding this comment.
I like this change. Makes throttle code more clear.
| Load() float64 | ||
| Store(rate float64) |
There was a problem hiding this comment.
Probably Set and Get are better names here.
There was a problem hiding this comment.
Does Get actually needed?
There was a problem hiding this comment.
Probably Set and Get are better names here.
underlying impl is using load & store, plus atomic.StoreInt64
There was a problem hiding this comment.
But this is abstraction which essentially provides a function which returns previously set value. Interface should not be bound to particular implementation.
Actually, do you really need these structs and interface? It seems just like one more layer of redirection w/o any logic.
There was a problem hiding this comment.
Interface should not be bound to particular implementation.
yep, but seems that load and store is more common?
ref: https://golang.org/pkg/sync/atomic/#LoadInt32
There was a problem hiding this comment.
Actually, do you really need these structs and interface? It seems just like one more layer of redirection w/o any logic.
unfortunately, matching is using this
| rl.Lock() | ||
| defer rl.Unlock() |
There was a problem hiding this comment.
| rl.Lock() | |
| defer rl.Unlock() | |
| rl.RLock() | |
| defer rl.RUnlock() |
and same for Burst (otherwise why to use RWMutex?)
There was a problem hiding this comment.
Why? RWMutex seems to suite better here.
There was a problem hiding this comment.
rw mutex is relatively slower, plus the use case is NOT multiple reader getting the rate & burst
| rateLimiter: quotas.NewDefaultIncomingDynamicRateLimiter( | ||
| func() float64 { return float64(config.RPS()) }, |
| rateLimiter: quotas.NewDefaultIncomingDynamicRateLimiter( | ||
| func() float64 { return float64(config.RPS()) }, |
There was a problem hiding this comment.
Conceptual question: why do history and matching need throttler at all? Shouldn't throttling to be handled on FE only?
There was a problem hiding this comment.
each individual host / service need to do proper self protection (according to the current resource usage)
There was a problem hiding this comment.
But who can produce such load besides FE? Yes there is matching <-> history communication, but they are not user facing and probably shouldn't be throttled.
There was a problem hiding this comment.
It would be great to add metric to see if history and matching throttles actually throttle.
There was a problem hiding this comment.
but they are not user facing and probably shouldn't be throttled.
then the host can be overloaded
2 participants
What changed?
Why?
Support burst for rate limiter & code refactoring
How did you test it?
Run existing tests
Potential risks
N/A