feat: introduced custom rate limiter based on options pattern #106
Conversation
aaronschweig
left a comment
aaronschweig
left a comment
There was a problem hiding this comment.
Left some question and review suggestions
| func (r *StaticThenExponentialRateLimiter[T]) When(item T) time.Duration { | ||
| r.failuresLock.Lock() | ||
| defer r.failuresLock.Unlock() | ||
|
|
||
| now := r.clock.Now() | ||
|
|
||
| first, exists := r.firstAttempt[item] | ||
| if !exists { | ||
| first = now | ||
| r.firstAttempt[item] = first | ||
| } | ||
|
|
||
| timeSinceFirst := now.Sub(first) | ||
| if timeSinceFirst <= r.staticWindow { | ||
| return r.staticDelay | ||
| } | ||
|
|
||
| return r.exponential.When(item) | ||
| } |
There was a problem hiding this comment.
I would appreciate a bit of a differentiation between the usage of read and write locks in here.
Especially since in this read case now, you are blocking the whole execution of other reads and writes. It would be good to only set the right type of lock where needed, and only set it as shortly as needed.
There was a problem hiding this comment.
Sure, thanks, it's done
|
|
||
| first, exists := r.firstAttempt[item] | ||
| if !exists { | ||
| first = now |
There was a problem hiding this comment.
I am not quite sure why this assignment is needed
There was a problem hiding this comment.
I've double checked the logic, and updated it. Currently the rate limiter will return the static delay for the first run and this assignment is not needed anymore
|
|
||
| type StaticThenExponentialRateLimiter[T comparable] struct { | ||
| failuresLock sync.Mutex | ||
| firstAttempt map[T]time.Time |
There was a problem hiding this comment.
Not quite sure I agree with the naming. I rather have something like staticAttempts or something similiar, as it cannot only hold the first attempt but rather as many attempts as needed until the staticWindow limitation is needed
There was a problem hiding this comment.
I thought that we should only store the first attempt and then check how long it has been since the first attempt and if this time has reached the staticWindow duration, then we should fall into exponential backoff. Does this make sense?
There was a problem hiding this comment.
I agree with the implementation, I just don't like the naming 😉
There was a problem hiding this comment.
ah, sorry, I misunderstood
On-behalf-of: SAP [email protected]
On-behalf-of: SAP [email protected]
|
I had also implemented rate limiting in a way less sophisticated way by back-porting what we did in metadata-operator in this branch: https://github.com/platform-mesh/golang-commons/tree/feat/rate-limiter and also applied it to our operator in a draft PR https://github.tools.sap/dxp/metadata-registry-operator/pull/1522 Not sure if we can join our forces or just skip mine in favor of what you are doing here. It is definitely better to have more options than my static time limiting approach :) |
On-behalf-of: SAP [email protected]
nexus49
force-pushed
the
feat/custom-rate-limiter
branch
from
6dd7f00 to
dd78274
Compare
…olang-commons into feat/custom-rate-limiter On-behalf-of: SAP [email protected]
On-behalf-of: SAP [email protected]
On-behalf-of: SAP [email protected]
On-behalf-of: SAP [email protected]
On-behalf-of: SAP [email protected]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
On-behalf-of: SAP [email protected]