Unverified Commit 01eb939a authored by zhaozhao.zz's avatar zhaozhao.zz Committed by GitHub
Browse files

update monitor client's memory and evict correctly (#12420)

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->type` and the type in `getClientType()` are confusing
(in the later, `CLIENT_TYPE_NORMAL` not `CLIENT_TYPE_SLAVE`), 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).
parent 9f512017
...@@ -3808,7 +3808,7 @@ size_t getClientMemoryUsage(client *c, size_t *output_buffer_mem_usage) { ...@@ -3808,7 +3808,7 @@ size_t getClientMemoryUsage(client *c, size_t *output_buffer_mem_usage) {
* classes of clients. * classes of clients.
* *
* The function will return one of the following: * The function will return one of the following:
* CLIENT_TYPE_NORMAL -> Normal client * CLIENT_TYPE_NORMAL -> Normal client, including MONITOR
* CLIENT_TYPE_SLAVE -> Slave * CLIENT_TYPE_SLAVE -> Slave
* CLIENT_TYPE_PUBSUB -> Client subscribed to Pub/Sub channels * CLIENT_TYPE_PUBSUB -> Client subscribed to Pub/Sub channels
* CLIENT_TYPE_MASTER -> The client representing our replication master. * CLIENT_TYPE_MASTER -> The client representing our replication master.
......
...@@ -610,6 +610,7 @@ void replicationFeedMonitors(client *c, list *monitors, int dictid, robj **argv, ...@@ -610,6 +610,7 @@ void replicationFeedMonitors(client *c, list *monitors, int dictid, robj **argv,
while((ln = listNext(&li))) { while((ln = listNext(&li))) {
client *monitor = ln->value; client *monitor = ln->value;
addReply(monitor,cmdobj); addReply(monitor,cmdobj);
updateClientMemUsageAndBucket(monitor);
} }
decrRefCount(cmdobj); decrRefCount(cmdobj);
} }
......
...@@ -917,11 +917,9 @@ void removeClientFromMemUsageBucket(client *c, int allow_eviction) { ...@@ -917,11 +917,9 @@ void removeClientFromMemUsageBucket(client *c, int allow_eviction) {
* together clients consuming about the same amount of memory and can quickly * together clients consuming about the same amount of memory and can quickly
* free them in case we reach maxmemory-clients (client eviction). * free them in case we reach maxmemory-clients (client eviction).
* *
* Note: This function filters clients of type monitor, master or replica regardless * Note: This function filters clients of type no-evict, master or replica regardless
* of whether the eviction is enabled or not, so the memory usage we get from these * of whether the eviction is enabled or not, so the memory usage we get from these
* types of clients via the INFO command may be out of date. If someday we wanna * types of clients via the INFO command may be out of date.
* improve that to make monitors' memory usage more accurate, we need to re-add this
* function call to `replicationFeedMonitors()`.
* *
* returns 1 if client eviction for this client is allowed, 0 otherwise. * returns 1 if client eviction for this client is allowed, 0 otherwise.
*/ */
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment