Skip to content

Conversation

@sarthakaggarwal97
Copy link
Contributor

@sarthakaggarwal97 sarthakaggarwal97 commented Dec 20, 2024

In this PR, we introduce a support for few more filters for CLIENT LIST and CLIENT KILL commands. We introduce these new filters:

  1. FLAGS Client must include this flag. This can be a string with bunch of flags present one after the other.
  2. NAME client name
  3. IDLE minimum idle time of the client
  4. LIB-NAME clients with the specified lib name.
  5. LIB-VER clients with the specified lib version.
  6. DB clients currently operating on the specified database ID
  7. IP client ip address
  8. CAPA client capabilities

Partly Addresses: #668

@sarthakaggarwal97 sarthakaggarwal97 marked this pull request as ready for review December 20, 2024 16:59
@sarthakaggarwal97 sarthakaggarwal97 force-pushed the client-extended-filters branch 3 times, most recently from 094ee75 to e518d93 Compare December 20, 2024 19:49
@codecov
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 71.05263% with 44 lines in your changes missing coverage. Please review.

Project coverage is 71.00%. Comparing base (51387dd) to head (4b7d0d7).
Report is 1 commits behind head on unstable.

Files with missing lines Patch % Lines
src/networking.c 71.05% 44 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1466      +/-   ##
============================================
+ Coverage     70.98%   71.00%   +0.01%     
============================================
  Files           123      123              
  Lines         65768    65912     +144     
============================================
+ Hits          46687    46799     +112     
- Misses        19081    19113      +32     
Files with missing lines Coverage Δ
src/commands.def 100.00% <ø> (ø)
src/networking.c 87.48% <71.05%> (-1.03%) ⬇️

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sarthakaggarwal97 sarthakaggarwal97 force-pushed the client-extended-filters branch 4 times, most recently from f7ad3a7 to e3fdff0 Compare January 15, 2025 22:14
@zuiderkwast zuiderkwast added the major-decision-pending Major decision pending by TSC team label Jan 15, 2025
@sarthakaggarwal97 sarthakaggarwal97 force-pushed the client-extended-filters branch 4 times, most recently from 92f685b to c843fb9 Compare January 22, 2025 00:26
@sarthakaggarwal97
Copy link
Contributor Author

@zuiderkwast @hpatro would you please help review this. I have added the additional filters requested in #668.
Once the decision is being taken over the IP filter, I can add that as well. Thank you!

@sarthakaggarwal97
Copy link
Contributor Author

@valkey-io/core-team requesting you to take a look at the PR. Related to #668! Thank you

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me on a high level. The refactoring is already done in the previous PR so the implementation in this PR strait-forward.

Still, it's a large change to the user-facing interface that needs to be carefully reviewed and needs majority approval. We are near the cutoff for 8.1 and we have many features that seem more important to prioritize. We may need to postpone this feature to 9.0, or maybe we can accept it after RC1.

@soloestoy
Copy link
Member

does this PR support negative filtering?

@sarthakaggarwal97
Copy link
Contributor Author

@soloestoy it doesn't support negative filtering right now. Would you recommend doing it in this PR itself?

@soloestoy
Copy link
Member

@soloestoy it doesn't support negative filtering right now. Would you recommend doing it in this PR itself?

that would be very appreciated :)

@sarthakaggarwal97
Copy link
Contributor Author

@soloestoy do we want negative filtering only for flags or every other filter? Also, how would you recommend the filter command should look like with negation?

Copy link
Collaborator

@hpatro hpatro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM.

@sarthakaggarwal97 sarthakaggarwal97 force-pushed the client-extended-filters branch from c843fb9 to 0ed63c7 Compare March 17, 2025 23:08
sarthakaggarwal97 and others added 12 commits April 14, 2025 13:50
Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
@sarthakaggarwal97 sarthakaggarwal97 force-pushed the client-extended-filters branch from dd7af47 to 4b7d0d7 Compare April 14, 2025 20:50
@hpatro hpatro merged commit be60586 into valkey-io:unstable Apr 14, 2025
58 of 59 checks passed
@github-project-automation github-project-automation bot moved this from Optional for next release candidate to Done in Valkey 9.0 Apr 14, 2025
@hpatro
Copy link
Collaborator

hpatro commented Apr 14, 2025

Great work @sarthakaggarwal97! It's merged now. 🎉

@hpatro
Copy link
Collaborator

hpatro commented Apr 14, 2025

@sarthakaggarwal97 Please submit the doc PR.

zuiderkwast pushed a commit to valkey-io/valkey-doc that referenced this pull request Apr 16, 2025
Documentation for additional filters introduced in
valkey-io/valkey#1466

Signed-off-by: Sarthak Aggarwal <[email protected]>
@zuiderkwast zuiderkwast removed the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Apr 16, 2025
@sarthakaggarwal97
Copy link
Contributor Author

sarthakaggarwal97 commented Apr 16, 2025

@hpatro Doc PR has been merged, thank you

nitaicaro pushed a commit to nitaicaro/valkey that referenced this pull request Apr 22, 2025
…1466)

In this PR, we introduce support for new filters for `CLIENT
LIST` and `CLIENT KILL` commands. The new filters are:

1. FLAGS `Client must include this flag. This can be a string with bunch
of flags present one after the other.`
2. NAME `client name`
3. IDLE `minimum idle time of the client`
4. LIB-NAME `clients with the specified lib name.`
5. LIB-VER `clients with the specified lib version.`
6. DB `clients currently operating on the specified database ID`
7. IP `client ip address`
8. CAPA `client capabilities`

Partly Addresses: valkey-io#668

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
Signed-off-by: Nitai Caro <[email protected]>
nitaicaro pushed a commit to nitaicaro/valkey that referenced this pull request Apr 22, 2025
…1466)

In this PR, we introduce support for new filters for `CLIENT
LIST` and `CLIENT KILL` commands. The new filters are:

1. FLAGS `Client must include this flag. This can be a string with bunch
of flags present one after the other.`
2. NAME `client name`
3. IDLE `minimum idle time of the client`
4. LIB-NAME `clients with the specified lib name.`
5. LIB-VER `clients with the specified lib version.`
6. DB `clients currently operating on the specified database ID`
7. IP `client ip address`
8. CAPA `client capabilities` 

Partly Addresses: valkey-io#668

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
nitaicaro pushed a commit to nitaicaro/valkey that referenced this pull request Apr 22, 2025
…1466)

In this PR, we introduce support for new filters for `CLIENT
LIST` and `CLIENT KILL` commands. The new filters are:

1. FLAGS `Client must include this flag. This can be a string with bunch
of flags present one after the other.`
2. NAME `client name`
3. IDLE `minimum idle time of the client`
4. LIB-NAME `clients with the specified lib name.`
5. LIB-VER `clients with the specified lib version.`
6. DB `clients currently operating on the specified database ID`
7. IP `client ip address`
8. CAPA `client capabilities` 

Partly Addresses: valkey-io#668

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
hwware pushed a commit to wuranxx/valkey that referenced this pull request Apr 24, 2025
…1466)

In this PR, we introduce support for new filters for `CLIENT
LIST` and `CLIENT KILL` commands. The new filters are:

1. FLAGS `Client must include this flag. This can be a string with bunch
of flags present one after the other.`
2. NAME `client name`
3. IDLE `minimum idle time of the client`
4. LIB-NAME `clients with the specified lib name.`
5. LIB-VER `clients with the specified lib version.`
6. DB `clients currently operating on the specified database ID`
7. IP `client ip address`
8. CAPA `client capabilities`

Partly Addresses: valkey-io#668

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
Signed-off-by: hwware <[email protected]>
@zuiderkwast zuiderkwast removed the to-be-merged Almost ready to merge label May 27, 2025
charsyam pushed a commit to charsyam/valkey that referenced this pull request Jun 3, 2025
…1466)

In this PR, we introduce support for new filters for `CLIENT
LIST` and `CLIENT KILL` commands. The new filters are:

1. FLAGS `Client must include this flag. This can be a string with bunch
of flags present one after the other.`
2. NAME `client name`
3. IDLE `minimum idle time of the client`
4. LIB-NAME `clients with the specified lib name.`
5. LIB-VER `clients with the specified lib version.`
6. DB `clients currently operating on the specified database ID`
7. IP `client ip address`
8. CAPA `client capabilities`

Partly Addresses: valkey-io#668

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
Signed-off-by: charsyam <[email protected]>
stockholmux added a commit to stockholmux/valkey that referenced this pull request Aug 13, 2025
Signed-off-by: Kyle J. Davis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

major-decision-approved Major decision approved by TSC team run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants