Fix delay behavior #660
Fix delay behavior #660
Conversation
| current := r.addRequests(uniqueKey, hits) | ||
| delay := time.Duration(resp.GetResetTime()-createdAt+int64(current)*cfg.ThrottleInterval.Milliseconds()) * time.Millisecond |
There was a problem hiding this comment.
@vigneshshanmugam Can I ask your opinion on this logic before I do the same for the local rate limiter?
This bug was added to GA goals: #619
This implementation has some things that worry me, but I don't if it makes sense to be worried over this:
- What if we receive tons of requests? The delay will be increasing for each request, and then we will have many processes on hold. I think it makes sense to maybe set the delay to a fixed time, and after that time, each request starts getting rejected, regardless of the strategy. WDYT?
- What if we receive requests like this:
- We get a request that takes 10 tokens
- We then get a request that takes 4 tokens
- Both are delayed. Should the request of 4 tokens take priority, thus less delay for it, or should the 10 tokens have priority since it arrived first?
There was a problem hiding this comment.
I dont think you need to do this, The issue here is that we are allowing all the clients to retry and send data simultanously once we past the reset time which is wrong. Its assuming the reset time guarentees the availability of tokens - requests/bytes based on the strategy.
The ideal way we could fix is basically looping through and retry after the delay, re checking the rate limits at each time. This respects all the configurations like strategy/algorithm along with keeping the race condition in check.
Rough sudo code would be
for {
// check rate limit
resp = getCurrentLimits()
if resp.IsUnderLimit {
// not limited
return nil
}
// we shouldn't use createAt here its flawed as its based on when the request is made and doesnt take retries in to account.
delay := time.Duration(resp.GetResetTime()-time.Now().UnixMilli()) * time.Millisecond
// same code as before
} Hope this helps. Let me know if you want more details.
| r.currentRequests[uniqueKey] = hits | ||
| } else { | ||
| r.currentRequests[uniqueKey] = current + hits | ||
| } |
There was a problem hiding this comment.
drive-by-comment: The entries for projects do not get cleaned up as long as there is no restart. This could lead to an amount of projects that are tested out for a bit, but then unused and kept in this map. Maybe an approach with using a TTL would be better? It might lead to active projects getting reset on TTL, but I think this would be fine to accept.
There was a problem hiding this comment.
Thanks @simitt , I also noticed that, but wanted to double check the logic first. I will look into this again once I complete the other GA task
Issue is #619.