valkey icon indicating copy to clipboard operation
valkey copied to clipboard

Introduce CLUSTER SLOT-STATS command (#20).

Open kyle-yh-kim opened this issue 1 year ago • 4 comments

The command provides detailed slot usage statistics upon invocation, with initial support for key-count metric. cpu-usec (approved) and memory-bytes (pending-approval) metrics will soon follow after the merger of this PR.

kyle-yh-kim avatar Apr 23 '24 02:04 kyle-yh-kim

Run:

git rebase HEAD~1 --signoff
git push origin --force 20-slotstats

To fix the DCO complaint.

madolson avatar Apr 23 '24 04:04 madolson

Thanks for sharing. Two changes added;

  • Fixed DCO complaint through --signoff
  • Added trailing new line for cluster_slots.c

kyle-yh-kim avatar Apr 30 '24 18:04 kyle-yh-kim

@PingXie I think you asked for this in the past, you should also take a look if you have time.

madolson avatar May 07 '24 23:05 madolson

Codecov Report

Attention: Patch coverage is 87.36842% with 12 lines in your changes missing coverage. Please review.

Project coverage is 70.12%. Comparing base (ce79539) to head (3cf86ab). Report is 10 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #351      +/-   ##
============================================
+ Coverage     70.05%   70.12%   +0.06%     
============================================
  Files           110      111       +1     
  Lines         60084    60203     +119     
============================================
+ Hits          42094    42218     +124     
+ Misses        17990    17985       -5     
Files Coverage Δ
src/commands.def 100.00% <ø> (ø)
src/server.h 100.00% <ø> (ø)
src/cluster_slot_stats.c 87.36% <87.36%> (ø)

... and 14 files with indirect coverage changes

codecov[bot] avatar May 07 '24 23:05 codecov[bot]

Review this during a separate meeting.

  1. High level approval for the approach of adding per-slot statistics.
  2. Start with CPU, key count, network bytes in, network bytes out.
  3. Defer the decision about memory usage since it was contentious.

Still need alignment about the command arguments. Will have separate approval for that.

madolson avatar May 27 '24 15:05 madolson

I've cut a 2nd revision, which includes;

  • Renamed cluster_slots.c to cluster_slot_stats.c
  • Renamed function signature and variables (ex: *slots --> *assigned_slots).
  • Removed 0 argument support (just CLUSTER SLOT-STATS).

@PingXie @madolson I did not update the existing bytes array into bitmaps, based on the performance testing result attached here. Unless there's a strong opinion and/or hard evidence otherwise, I'd prefer moving forward with what we have now.

kyle-yh-kim avatar Jun 18 '24 15:06 kyle-yh-kim

@kyle-yh-kim Can you do a git merge and resolve the conflicts. Also please open a PR like https://github.com/valkey-io/valkey-doc/pull/143 to add documentation for it.

madolson avatar Jun 24 '24 18:06 madolson

PR for Valkey-doc has been opened; https://github.com/valkey-io/valkey-doc/pull/150

kyle-yh-kim avatar Jun 25 '24 22:06 kyle-yh-kim