Use fake client flag to replace not conn check #1198
Conversation
The fake client flag was introduced in valkey-io#1063, we want this to replace all !conn fake client checks. Signed-off-by: Binbin <[email protected]>
| @@ -894,7 +894,7 @@ void updateClientMemoryUsage(client *c) { | |||
| } | |||
|
|
|||
| int clientEvictionAllowed(client *c) { | |||
| if (server.maxmemory_clients == 0 || c->flag.no_evict || !c->conn) { | |||
| if (server.maxmemory_clients == 0 || c->flag.no_evict || c->flag.fake) { | |||
| return 0; | |||
| } | |||
| int type = getClientType(c); | |||
There was a problem hiding this comment.
This is good practice. But how do we ensure a new dev/reviewer ensure(s) this? Wouldn't this be better asserted while setting/unsetting the flag in one place? WDYT?
There was a problem hiding this comment.
Wouldn't this be better asserted while setting/unsetting the flag in one place?
I think we would need to create a wrapper like the below and make flag.fake a client internal state.
connection *clientGetConnection(client *c) {
serverAssert((c->flag.fake && !c->conn) || (!c->flag.fake && c->conn));
return c->conn;
}There was a problem hiding this comment.
the wrapper changed will require a lot of code changes, there a LOT OF code are using c->conn
There was a problem hiding this comment.
@enjoy-binbin We merge this and file a new issue with help-wanted/good-first-issue tag?
There was a problem hiding this comment.
I'm not worried about not doing it myself, i just wonder if it's worth doing (diff causes conflicts).
But looking at the code again, i guess doing this wrapper is indeed a good way to avoid fake client connection issue in the future. I will do it in a later commit.
There was a problem hiding this comment.
I'm starting to feel like we might still need to keep c->conn...
Signed-off-by: Binbin <[email protected]>
| @@ -894,7 +894,7 @@ void updateClientMemoryUsage(client *c) { | |||
| } | |||
|
|
|||
| int clientEvictionAllowed(client *c) { | |||
| if (server.maxmemory_clients == 0 || c->flag.no_evict || !c->conn) { | |||
| if (server.maxmemory_clients == 0 || c->flag.no_evict || c->flag.fake) { | |||
| return 0; | |||
| } | |||
| int type = getClientType(c); | |||
There was a problem hiding this comment.
Wouldn't this be better asserted while setting/unsetting the flag in one place?
I think we would need to create a wrapper like the below and make flag.fake a client internal state.
connection *clientGetConnection(client *c) {
serverAssert((c->flag.fake && !c->conn) || (!c->flag.fake && c->conn));
return c->conn;
}
|
i think the change somehow break the CI, i will take a deep look later. |
|
ok, i see the test fail because we have a createCachedResponseClient, it is a fake client but with a connection |
Signed-off-by: Binbin <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1198 +/- ##
============================================
+ Coverage 70.65% 70.66% +0.01%
============================================
Files 114 114
Lines 61799 63156 +1357
============================================
+ Hits 43664 44632 +968
- Misses 18135 18524 +389
|
The fake client flag was introduced in #1063,
we want this to replace all !conn fake client checks.