update monitor client's memory and evict correctly#12420
update monitor client's memory and evict correctly#12420oranagra merged 1 commit intoredis:unstablefrom
Conversation
|
@soloestoy Sorry for my bad, you are right, thanks for figuring it out. |
don't worry, it's harmless 😄 |
how is that different than what we had in 6.X? |
There was a problem hiding this comment.
so because the type in c->type and the type in getClientType are confusing, the comment we wrote in updateClientMemUsageAndBucket was wrong, and in fact that function didn't skip monitor clients.
and since it doesn't skip monitor clients, it was wrong to delete the call for it from replicationFeedMonitors (it wasn't a NOP).
that deletion could mean that the monitor client memory usage is not always up to date (updated less frequently, but still a candidate for client eviction).
since it's just monitors (not a critical feature), with client eviction (off by default), it's not a critical bug IMHO.
however, now i wonder what's the right fix, maybe instead of reverting the change in replicationFeedMonitors and updating the comment in updateClientMemUsageAndBucket, we need to keep that comment and change the code so that it is true.
i.e. maybe ignoring monitors the same way as we ignore replicas is the right behavior?
same as replicas, monitors accumulate output buffers as a result of other clients work (or abuse), so disconnecting the monitor will not stop that work (or abuse).
but on the other hand, one possible way the monitor can accumulate a large output buffer is if the client doesn't read the data from the socket fast enough. and indeed disconnecting the monitor will release the memory.
I tend to agree with this PR, just want to make sure we fix it the right way this time.
WDYT?
|
I perceive |
A bug introduced in #11657 (7.2 RC1), causes client-eviction (#8687) and INFO to have inaccurate memory usage metrics of MONITOR clients.
Because the type in
c->typeand the type ingetClientType()are confusing(in the later,
CLIENT_TYPE_NORMALnotCLIENT_TYPE_SLAVE), the commentwe wrote in
updateClientMemUsageAndBucketwas wrong, and in fact that functiondidn't skip monitor clients.
And since it doesn't skip monitor clients, it was wrong to delete the call for it from
replicationFeedMonitors(it wasn't a NOP).That deletion could mean that the monitor client memory usage is not always up to
date (updated less frequently, but still a candidate for client eviction).