feat: CachedRedisStorage will now default to flushing a batch ASAP to… #294
Conversation
… reduce the length of time the cache is out of sync with Redis counters. Setting a flushing_period is still supported to have a delay before flushing which can be useful to reduce the load on the Redis server in exchange for having staler counters in limitador. Signed-off-by: Hiram Chirino <[email protected]>
| // Wait for a new flush request, | ||
| flush_rx.changed().await.unwrap(); | ||
|
|
||
| if flushing_period != Duration::ZERO { | ||
| // Set the flushing_period to reduce the load on Redis the downside to | ||
| // setting it the cached counters will not be as accurate as when it's zero. | ||
| tokio::time::sleep(flushing_period).await | ||
| } |
There was a problem hiding this comment.
Have you looked at tokio's select!? We could have either wait for the period to elapse or wait to be notified... So that you'd flush every n seconds or flush when notified. Cause this would have the flush task wait after being notified.
There was a problem hiding this comment.
Waiting after the first notification is a feature. As it's acting as a debounce timeout. Helping you batch and dedup counter updates in a short spurt of activity.
There was a problem hiding this comment.
See here... I don't know how much of an added value that notification is useful really... What do you think we're addressing by introducing this behavior? Other than the loop as fast as possible with the 0 flushing period... This "just" changes what the starting point of a flushing period is... 😕 What am I missing?
There was a problem hiding this comment.
The tokio::select! would be useful if we want to flush when whatever happens first, either the notify() or the flushing_period.
| @@ -44,6 +44,7 @@ pub struct CachedRedisStorage { | |||
| async_redis_storage: AsyncRedisStorage, | |||
| redis_conn_manager: ConnectionManager, | |||
| partitioned: Arc<AtomicBool>, | |||
| flush_tx: tokio::sync::watch::Sender<()>, | |||
There was a problem hiding this comment.
Wondering if a Notify wouldn't be more appropriate, correct me if I'm wrong, but this will enqueue every single change and have the loop on "the other side" redo all the work... while it'd race against updates in the shared DS...
There was a problem hiding this comment.
Yeah it does look simpler. Watch channels don't enqueue, they only store the last value.
There was a problem hiding this comment.
Ah, right, they do. And Notify sorts of behave similarly. So that now... with the timeout being 0 this will probably loop like crazy, other than the system being idle. The batch will be of the size of the race. So thinking we should notify "smartly"... wdyt?
There was a problem hiding this comment.
Updated the PR to use Notify.
There was a problem hiding this comment.
Correct: it will keep flushing ASAP while there there is a pending increment. This will converge the cache with the global state quicker.
with the timeout being
0this will probably loop like crazy, other than the system being idle.
| @@ -174,6 +175,9 @@ impl AsyncCounterStorage for CachedRedisStorage { | |||
| } | |||
| } | |||
|
|
|||
| // ask the flusher to flush the batch | |||
| self.flush_tx.send(()).unwrap(); | |||
There was a problem hiding this comment.
I like this a lot, but would make it conditional on ... being within a certain threshold of the limit? e.g. close to expiry and/or "reaching" the limit?
There was a problem hiding this comment.
What's the benefit of delaying the flush? I can see the following benefits:
- Reduce the number Redis API calls since you group multiple calls into 1 batch
- Can dedupe multiple increments of the same counter
Both of the above will likely reduce CPU load on Redi s. But that's not the main problem global rate limiters are solving for. The main problem is high latency calls to Redis. Adding a delay only increases that latency further.
More latency means the local cache counters will not have the correct global value.
I do think if the load is large enough CPU load on redis will be a problem, so keeping the flushing_period option is good since that would introduce that delay that can help reduce the load on Redis. But again, I don't think that's the main problem we are solving for.
Signed-off-by: Hiram Chirino <[email protected]>
|
I think we can close this one, right? |
|
yep not need any more. |
… reduce the length of time the cache is out of sync with Redis counters.
Setting a flushing_period is still supported to have a delay before flushing which can be useful to reduce the load on the Redis server in exchange for having staler counters in limitador.