Skip to content

Conversation

@soloestoy
Copy link
Member

Allow users to more easily get cluster information.

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.

I'm fine with this.

The only risk I would like to call out is we have been wary of increasing the info and info all command response size to avoid tail latency for other commands.

@hpatro hpatro requested a review from a team November 26, 2025 18:48
Copy link
Member

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

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

1c708b2

Seems ok to me. Does anyone know the history? why we have this cluster section and have the CLUSTER INFO command.

@zuiderkwast zuiderkwast requested a review from ranshid November 27, 2025 08:38
Copy link
Member

@ranshid ranshid left a comment

Choose a reason for hiding this comment

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

I have the same concern as @hpatro, since this would increase the latency when the cluster has a large number of nodes.
However, since info all command already performs horribly and the best practice is to explicitly define the sections of interest to avoid potential degradation I think this is fine

@ranshid ranshid added release-notes This issue should get a line item in the release notes cluster labels Nov 30, 2025
@ranshid
Copy link
Member

ranshid commented Nov 30, 2025

I have the same concern as @hpatro, since this would increase the latency when the cluster has a large number of nodes.
However, since

1c708b2

Seems ok to me. Does anyone know the history? why we have this cluster section and have the CLUSTER INFO command.

I can only imagine that it was meant to avoid bloating the already extensive info output.
Another thing is that it might be related to security? I mean in some cases it might be harder to prevent exposing cluster internal stats (even though I can hardly imagine how can this be exploited)

@codecov
Copy link

codecov bot commented Dec 4, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.45%. Comparing base (da3c43d) to head (959c889).
⚠️ Report is 18 commits behind head on unstable.

Files with missing lines Patch % Lines
src/debug.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2876      +/-   ##
============================================
+ Coverage     72.43%   72.45%   +0.02%     
============================================
  Files           128      128              
  Lines         70439    70439              
============================================
+ Hits          51022    51038      +16     
+ Misses        19417    19401      -16     
Files with missing lines Coverage Δ
src/cluster.c 90.46% <100.00%> (+0.26%) ⬆️
src/cluster_legacy.c 87.43% <100.00%> (-0.15%) ⬇️
src/server.c 88.55% <100.00%> (+0.14%) ⬆️
src/debug.c 54.63% <0.00%> (ø)

... and 14 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.

@ranshid ranshid merged commit 04d0bba into valkey-io:unstable Dec 4, 2025
57 of 58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cluster release-notes This issue should get a line item in the release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants