-
Notifications
You must be signed in to change notification settings - Fork 955
Adding Missing filters to CLIENT LIST and Dedup Parsing #1401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding Missing filters to CLIENT LIST and Dedup Parsing #1401
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1401 +/- ##
============================================
- Coverage 70.94% 70.93% -0.01%
============================================
Files 120 120
Lines 65044 65078 +34
============================================
+ Hits 46143 46161 +18
- Misses 18901 18917 +16
|
54adfa6 to
7630eba
Compare
zuiderkwast
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
I just took a quick look and I can see some things that appear to be broken:
- CLIENT LIST with multiple IDs, for example
CLIENT LIST ID 3 4 5. (It means id is 3 or 4 or 5.) We a test case for this. - The default for "skipme" for CLIENT KILL should be YES by default (when filters are used), but for CLIENT LIST, skipme should be NO by default. Otherwise, we have a breaking change. Please add some tests without SKIPME but with other filters matching the calling client to cover the default skipme value for LIST and KILL.
We need some test cases combining multiple filter rules at the same time.
|
Thanks @zuiderkwast for the comments! I am working on them |
049f2d4 to
e3c1c5a
Compare
|
@zuiderkwast thank you for your comments. Adressed most of them. Working on adding tests meanwhile. Trying to see how can I have tests with multiple clients. Sharing some responses from local testing Client Kill Command Client List Command |
zuiderkwast
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to see how can I have tests with multiple clients.
You can create multiple clients.
set c1 [valkey_client]
set c2 [valkey_client]
set c3 [valkey_client]
...
$c1 close
$c2 close
$c3 close
fd743fe to
6681e47
Compare
zuiderkwast
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refactoring looks good. I have just a few small comments and there is some merge conflict.
6681e47 to
52fd611
Compare
52fd611 to
ac3f5e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just two nits.
|
@valkey-io/core-team Please approve the extended syntax for CLIENT LIST as described in the top comment. The MAXAGE argument for CLIENT LIST is a little strange, because it's the max age of clients to exclude, for symmetry with CLIENT KILL. An alternative is to forbid MAXAGE for CLIENT LIST. |
3b789c2 to
a944e0e
Compare
2120e61 to
301993f
Compare
|
Thanks folks for the suggestions and inputs. Based on @hwware suggestions, I performed the SET/GET operations while running Sharing the results:
Notes:
|
|
@sarthakaggarwal97 Thanks for sharing the results and is inline with our expectation. I'm good with the stats. The above benchmarking is performed with one client running the |
|
It looks good to me. Approved |
|
I see atleast 4 core-team member have approved this either through voting or PR approval. @valkey-io/core-team Do you want to review further or is it good to be merged? |
|
Let's merge it. @sarthakaggarwal97 do you want to update the docs too? The |
|
Thank you everyone for helping in this one. @zuiderkwast let me raise the doc PR as well. Thank you! |

Adds filter options to CLIENT LIST:
Modifies the ID filter to CLIENT KILL to allow multiple IDs
This makes CLIENT LIST and CLIENT KILL accept the same options.
For backward compatibility, the default value for SKIPME is NO for CLIENT LIST and YES for CLIENT KILL.
The MAXAGE comes from CLIENT KILL, where it keeps clients with the given max age and kills the older ones. This logic becomes weird for CLIENT LIST, but is kept for similary with CLIENT KILL, for the use case of first testing manually using CLIENT LIST, and then running CLIENT KILL with the same filters.
The
ID client-id [client-id ...]no longer needs to be the last filter. The parsing logic determines if an argument is an ID or not based on whether it can be parsed as an integer or not.Partly addresses: #668