Skip to content

add paused_actions for INFO Clients #1519

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
Merged
merged 5 commits into from
Jan 14, 2025
Merged

add paused_actions for INFO Clients #1519

merged 5 commits into from
Jan 14, 2025

Conversation

@soloestoy
Copy link
Member

@soloestoy soloestoy commented Jan 7, 2025 •
edited
Loading

Add paused_actions and paused_timeout_milliseconds for INFO Clients to inform users about if clients are paused.

@codecov
Copy link

codecov bot commented Jan 7, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.95%. Comparing base (b3b4bdc) to head (035d8a5).
Report is 31 commits behind head on unstable.

Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #1519      +/-   ##
============================================
+ Coverage     70.83%   70.95%   +0.11%     
============================================
  Files           120      120              
  Lines         64911    65060     +149     
============================================
+ Hits          45982    46164     +182     
+ Misses        18929    18896      -33
Files with missing lines Coverage Δ
src/networking.c 88.51% <100.00%> (+0.26%) ⬆️
src/server.c 87.64% <100.00%> (+0.17%) ⬆️
src/server.h 100.00% <ø> (ø)

... and 53 files with indirect coverage changes

@madolson madolson added the major-decision-pending Major decision pending by TSC team label Jan 7, 2025
madolson
madolson reviewed Jan 7, 2025
View reviewed changes
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

I think this is a good info field to add.

madolson
madolson approved these changes Jan 7, 2025
View reviewed changes
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

Just fix the format error and it's good for me.

hpatro
hpatro reviewed Jan 8, 2025
View reviewed changes
artikell
artikell reviewed Jan 8, 2025
View reviewed changes
@hwware
Copy link
Member

hwware commented Jan 8, 2025

I am confused why this pr is tagged as major-decision-pending?

@madolson
Copy link
Member

madolson commented Jan 9, 2025

@hwware New info fields. I thought I posted a message, but maybe I forgot. @valkey-io/core-team I suppose the major decision is if people are okay with the new info fields listed. (Maybe also including the one @artikell suggested).

@zuiderkwast
Copy link
Contributor

zuiderkwast commented Jan 9, 2025

This field will be "none" most of the time, right? If we add many fields like this, is the size of the INFO reply a concern?

I remember for the graceful shutdown feature, we added a field that is only present during shutdown:

  • shutdown_in_milliseconds: The maximum time remaining for replicas to catch up the replication before completing the shutdown sequence. This field is only present during shutdown.

In the same way, I wanted to add the availability_zone in HELLO reply only if it's non-empty, to avoid bloating the reply with a field that's always empty for many users.

@enjoy-binbin
Copy link
Member

enjoy-binbin commented Jan 9, 2025 •
edited
Loading

this make sense to me, i also mention this in #1131 (comment). I would also want a purpose field if we add the action.

@hwware
Copy link
Member

hwware commented Jan 10, 2025

@valkey-io/core-team Although it is approved, status is still major-decision pending, can we just have a vote for it?

@soloestoy
clang format
31153d2 
Signed-off-by: zhaozhao.zz <[email protected]>
@soloestoy
add paused_timeout
bce5c7c 
Signed-off-by: zhaozhao.zz <[email protected]>
@soloestoy soloestoy added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Jan 13, 2025
@zuiderkwast
Copy link
Contributor

zuiderkwast commented Jan 13, 2025

The "stats" section is maybe wrong? Isn't it better to put it in another INFO section?

@soloestoy
Copy link
Member Author

soloestoy commented Jan 14, 2025

@zuiderkwast which section you think is better?

@soloestoy
add unit for paused timeout
29e5c74 
Signed-off-by: zhaozhao.zz <[email protected]>
@zuiderkwast
Copy link
Contributor

zuiderkwast commented Jan 14, 2025

Clients section is better? Paused is about paused clients.

Stats is counters of various information. paused_actions is just the current state, so not really stats.

@soloestoy
Copy link
Member Author

soloestoy commented Jan 14, 2025

Clients makes sense, since it is from CLIENT PAUSE command

@soloestoy soloestoy changed the title add paused_actions for INFO stats add paused_actions for INFO Clients Jan 14, 2025
@soloestoy soloestoy added release-notes This issue should get a line item in the release notes needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. labels Jan 14, 2025
@soloestoy soloestoy merged commit c5a1585 into valkey-io:unstable Jan 14, 2025
50 checks passed
@soloestoy soloestoy mentioned this pull request Jan 14, 2025
Merged
@madolson
Copy link
Member

madolson commented Jan 14, 2025

I was going to suggest client or server, since it's server side state. I don't feel strongly to change it, I also agreed I thought stats was the wrong place.

enjoy-binbin added a commit to enjoy-binbin/valkey that referenced this pull request Jan 15, 2025
@enjoy-binbin
Add paused_purpose to INFO CLIENTS
cf2a680 
In valkey-io#1519, we added paused_actions and paused_timeout_milliseconds,
it would be helpful if we add the paused_purpose since users also
want to know the purpose for the pause.

Signed-off-by: Binbin <[email protected]>
@enjoy-binbin enjoy-binbin mentioned this pull request Jan 15, 2025
Merged
proost pushed a commit to proost/valkey that referenced this pull request Jan 17, 2025
@soloestoy @proost
add paused_actions for INFO Clients (valkey-io#1519)
32c57b5 
Add `paused_actions` and `paused_timeout_milliseconds` for INFO Clients
to inform users about if clients are paused.

---------

Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: proost <[email protected]>
kronwerk pushed a commit to kronwerk/valkey that referenced this pull request Jan 27, 2025
@soloestoy @kronwerk
add paused_actions for INFO Clients (valkey-io#1519)
af8927a 
Add `paused_actions` and `paused_timeout_milliseconds` for INFO Clients
to inform users about if clients are paused.

---------

Signed-off-by: zhaozhao.zz <[email protected]>
zuiderkwast pushed a commit that referenced this pull request Feb 6, 2025
@enjoy-binbin
Add paused_reason to INFO CLIENTS (#1564)
da3f1c6 
In #1519, we added paused_actions and paused_timeout_milliseconds,
it would be helpful if we add the paused_purpose since users also
want to know the purpose for the pause.

Currently available options:
- client_pause: trigger by CLIENT PAUSE command.
- shutdown_in_progress: during shutdown, primary waits the replicas to
catch up the offset.
- failover_in_progress: during failover, primary waits the replica to
catch up the offset.
- none

---------

Signed-off-by: Binbin <[email protected]>
enjoy-binbin added a commit to valkey-io/valkey-doc that referenced this pull request Feb 7, 2025
@soloestoy @enjoy-binbin @zuiderkwast
Add paused_actions and paused_timeout_milliseconds fields for info cl…
3e12e79 
…ients (#215)

See valkey-io/valkey#1519.

Signed-off-by: zhaozhao.zz <[email protected]>
Co-authored-by: Binbin <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
xbasel pushed a commit to xbasel/valkey that referenced this pull request Mar 27, 2025
@enjoy-binbin @xbasel
Add paused_reason to INFO CLIENTS (valkey-io#1564)
0b8cfa7 
In valkey-io#1519, we added paused_actions and paused_timeout_milliseconds,
it would be helpful if we add the paused_purpose since users also
want to know the purpose for the pause.

Currently available options:
- client_pause: trigger by CLIENT PAUSE command.
- shutdown_in_progress: during shutdown, primary waits the replicas to
catch up the offset.
- failover_in_progress: during failover, primary waits the replica to
catch up the offset.
- none

---------

Signed-off-by: Binbin <[email protected]>
xbasel pushed a commit to xbasel/valkey that referenced this pull request Mar 27, 2025
@enjoy-binbin @xbasel
Add paused_reason to INFO CLIENTS (valkey-io#1564)
83b1136 
In valkey-io#1519, we added paused_actions and paused_timeout_milliseconds,
it would be helpful if we add the paused_purpose since users also
want to know the purpose for the pause.

Currently available options:
- client_pause: trigger by CLIENT PAUSE command.
- shutdown_in_progress: during shutdown, primary waits the replicas to
catch up the offset.
- failover_in_progress: during failover, primary waits the replica to
catch up the offset.
- none

---------

Signed-off-by: Binbin <[email protected]>
murphyjacob4 pushed a commit to enjoy-binbin/valkey that referenced this pull request Apr 13, 2025
@enjoy-binbin @murphyjacob4
Add paused_reason to INFO CLIENTS (valkey-io#1564)
b515b10 
In valkey-io#1519, we added paused_actions and paused_timeout_milliseconds,
it would be helpful if we add the paused_purpose since users also
want to know the purpose for the pause.

Currently available options:
- client_pause: trigger by CLIENT PAUSE command.
- shutdown_in_progress: during shutdown, primary waits the replicas to
catch up the offset.
- failover_in_progress: during failover, primary waits the replica to
catch up the offset.
- none

---------

Signed-off-by: Binbin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hpatro hpatro hpatro left review comments

@madolson madolson madolson approved these changes

+1 more reviewer

@artikell artikell artikell left review comments

Reviewers whose approvals may not affect merge requirements

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

Valkey 8.1
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@soloestoy @hwware @madolson @zuiderkwast @enjoy-binbin @artikell @hpatro