Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add new flag in CLIENT LIST for import-source client #1398

Merged
merged 4 commits into from
Dec 10, 2024
Merged

Add new flag in CLIENT LIST for import-source client #1398

merged 4 commits into from
Dec 10, 2024

Conversation

lyq2333
Copy link
Contributor

@lyq2333 lyq2333 commented Dec 6, 2024 •
edited by zuiderkwast
Loading

Fixes #1350 and #1185 (comment).

  • Add new flag "I" in CLIENT LIST for import-source client
  • Add DEBUG_CONFIG for import-mode
  • Allow import-source status to be turned off when import-mode is off

@codecov Codecov
Copy link

codecov bot commented Dec 6, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.87%. Comparing base (6df376d) to head (83792d0).
Report is 11 commits behind head on unstable.

Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #1398      +/-   ##
============================================
+ Coverage     70.82%   70.87%   +0.04%     
============================================
  Files           118      118              
  Lines         63561    63592      +31     
============================================
+ Hits          45020    45071      +51     
+ Misses        18541    18521      -20
Files with missing lines Coverage Δ
src/config.c 77.63% <ø> (ø)
src/networking.c 88.61% <100.00%> (+0.08%) ⬆️

... and 14 files with indirect coverage changes

zuiderkwast
zuiderkwast reviewed Dec 6, 2024
View reviewed changes
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 in general. Thanks!

@zuiderkwast
Copy link
Contributor

zuiderkwast commented Dec 6, 2024 •
edited
Loading

Should I add a new option for CLIENT KILL, i.e. CLIENT KILL FLAG?

It's a good idea, but let's move this idea to #668.

@lyq2333 @zuiderkwast
Apply suggestions from code review
3d514d1 
Co-authored-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Yanqi Lv <[email protected]>
@lyq2333
Copy link
Contributor Author

lyq2333 commented Dec 9, 2024

Should I add a new option for CLIENT KILL, i.e. CLIENT KILL FLAG?

It's a good idea, but let's move this idea to #668.

Thanks for your review and suggestion. I like this idea 😄

@lyq2333 lyq2333 mentioned this pull request Dec 9, 2024
Open
zuiderkwast
zuiderkwast approved these changes Dec 9, 2024
View reviewed changes
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.

LGTM, thanks!

@valkey-io/core-team please approve the interface changes mentioned in the top comment.

@zuiderkwast zuiderkwast added release-notes This issue should get a line item in the release notes major-decision-pending Major decision pending by TSC team needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. labels Dec 9, 2024
enjoy-binbin
enjoy-binbin approved these changes Dec 9, 2024
View reviewed changes
@lyq2333 @enjoy-binbin
Update tests/unit/expire.tcl
83792d0 
Co-authored-by: Binbin <[email protected]>
Signed-off-by: Yanqi Lv <[email protected]>
@madolson madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Dec 10, 2024
@zuiderkwast zuiderkwast merged commit f951a1c into valkey-io:unstable Dec 10, 2024
48 checks passed
@zuiderkwast
Copy link
Contributor

@lyq2333 Can you open a doc PR to add the flag here: https://github.com/valkey-io/valkey-doc/blob/main/commands/client-list.md

@lyq2333
Copy link
Contributor Author

lyq2333 commented Dec 11, 2024

@lyq2333 Can you open a doc PR to add the flag here: https://github.com/valkey-io/valkey-doc/blob/main/commands/client-list.md

OK, I will include it in the introduction of import mode.

vudiep411 pushed a commit to Autxmaton/valkey that referenced this pull request Dec 15, 2024
@lyq2333 @zuiderkwast
@enjoy-binbin @vudiep411
Add new flag in CLIENT LIST for import-source client (valkey-io#1398)
61f4b6e 
- Add new flag "I" in `CLIENT LIST` for import-source client
- Add `DEBUG_CONFIG` for import-mode
- Allow import-source status to be turned off when import-mode is off

Fixes valkey-io#1350 and
valkey-io#1185 (comment).

---------

Signed-off-by: lvyanqi.lyq <[email protected]>
Signed-off-by: Yanqi Lv <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
Co-authored-by: Binbin <[email protected]>
@lyq2333 lyq2333 mentioned this pull request Dec 27, 2024
Merged
zuiderkwast added a commit to valkey-io/valkey-doc that referenced this pull request Jan 2, 2025
@lyq2333 @zuiderkwast
Introduce import-mode and client import-source (#205)
7a4c92a 
This PR is for valkey-io/valkey#1185 and
valkey-io/valkey#1398.

---------

Signed-off-by: lvyanqi.lyq <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zuiderkwast zuiderkwast zuiderkwast approved these changes

@enjoy-binbin enjoy-binbin enjoy-binbin approved these changes

@soloestoy soloestoy soloestoy approved these changes

@madolson madolson madolson approved these changes

@PingXie PingXie Awaiting requested review from PingXie

@hpatro hpatro Awaiting requested review from hpatro

Assignees
No one assigned
Labels
major-decision-approved Major decision approved by TSC team needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. release-notes This issue should get a line item in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fail CLIENT IMPORT-SOURCE When Import Mode is Disabled and Reflect Client Import Source State in CLIENT LIST
5 participants
@lyq2333 @zuiderkwast @enjoy-binbin @soloestoy @madolson