Introduce CLUSTER SLOT-STATS command (#20). #351

kyle-yh-kim · 2024-04-23T02:29:16Z

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.

madolson · 2024-04-23T04:38:12Z

Run:

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

To fix the DCO complaint.

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. Signed-off-by: Kyle Kim <[email protected]>

kyle-yh-kim · 2024-04-30T18:00:54Z

Thanks for sharing. Two changes added;

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

madolson · 2024-05-07T23:30:26Z

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

codecov · 2024-05-07T23:41:13Z

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

madolson

I remember most of this, and it still mostly LGTM.

src/cluster_slots.c

madolson · 2024-05-27T15:02:38Z

Review this during a separate meeting.

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

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

src/cluster_slots.c

src/commands/cluster-slot-stats.json

- Renamed cluster_slots.c to cluster_slot_stats.c - Renamed function signature and variables. - Removed 0 argument support. Signed-off-by: Kyle Kim <[email protected]>

kyle-yh-kim · 2024-06-18T15:23:46Z

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.

src/commands/cluster-slot-stats.json

src/cluster_slot_stats.c

- Update reply_schema. Signed-off-by: Kyle Kim <[email protected]>

madolson · 2024-06-24T18:25:24Z

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

src/commands/cluster-slot-stats.json

src/cluster_slot_stats.c

src/commands/cluster-slot-stats.json

Signed-off-by: Kyle Kim <[email protected]>

- Fixed origin/unstable merge conflicts. - Updated formatting. - Updated cluster-slot-stats.json. Signed-off-by: Kyle Kim <[email protected]>

src/Makefile

- Fixed format based on clang-format. - Re-added crccombine.o into Makefile. Signed-off-by: Kyle Kim <[email protected]>

- Valkey PR link; valkey-io/valkey#351. Signed-off-by: Kyle Kim <[email protected]>

kyle-yh-kim · 2024-06-25T22:11:29Z

PR for Valkey-doc has been opened; valkey-io/valkey-doc#150

- Valkey PR link; valkey-io/valkey#351. Signed-off-by: Kyle Kim <[email protected]>

- Updated RESP reply from map to array. - Renamed slotStatEntry to slotStatForSort. Signed-off-by: Kyle Kim <[email protected]>

src/cluster_slot_stats.c

- Updated the RESP3 response from array of maps to array of arrays. Signed-off-by: Kyle Kim <[email protected]>

src/cluster_slot_stats.c

src/commands/cluster-slot-stats.json

Signed-off-by: Madelyn Olson <[email protected]>

src/commands/cluster-slot-stats.json

Signed-off-by: Madelyn Olson <[email protected]>

src/cluster_slot_stats.c

kyle-yh-kim mentioned this pull request Apr 23, 2024

[NEW] Introduce slot level metrics to Valkey cluster #20

Closed

madolson added the major-decision-pending Major decision pending by TSC team label Apr 23, 2024

kyle-yh-kim force-pushed the 20-slotstats branch from 605c923 to e17019b Compare April 30, 2024 17:58

madolson reviewed May 7, 2024

View reviewed changes

madolson added major-decision-approved Major decision approved by TSC team major-decision-pending Major decision pending by TSC team and removed major-decision-pending Major decision pending by TSC team major-decision-approved Major decision approved by TSC team labels May 27, 2024

PingXie reviewed Jun 3, 2024

View reviewed changes

soloestoy reviewed Jun 17, 2024

View reviewed changes

src/commands/cluster-slot-stats.json Outdated Show resolved Hide resolved

Minor revision.

293fe67

- Renamed cluster_slots.c to cluster_slot_stats.c - Renamed function signature and variables. - Removed 0 argument support. Signed-off-by: Kyle Kim <[email protected]>

hwware reviewed Jun 19, 2024

View reviewed changes

src/commands/cluster-slot-stats.json Show resolved Hide resolved

hwware reviewed Jun 19, 2024

View reviewed changes

src/cluster_slot_stats.c Show resolved Hide resolved

madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Jun 20, 2024

Minor revision.

494af2c

- Update reply_schema. Signed-off-by: Kyle Kim <[email protected]>

madolson 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 Jun 24, 2024

madolson reviewed Jun 24, 2024

View reviewed changes

src/commands/cluster-slot-stats.json Outdated Show resolved Hide resolved

src/commands/cluster-slot-stats.json Outdated Show resolved Hide resolved

src/cluster_slot_stats.c Outdated Show resolved Hide resolved

src/commands/cluster-slot-stats.json Outdated Show resolved Hide resolved

kyle-yh-kim added 2 commits June 24, 2024 18:48

Merge branch 'unstable' into 20-slotstats

2768f38

Signed-off-by: Kyle Kim <[email protected]>

Minor revision.

5f34353

- Fixed origin/unstable merge conflicts. - Updated formatting. - Updated cluster-slot-stats.json. Signed-off-by: Kyle Kim <[email protected]>

madolson reviewed Jun 24, 2024

View reviewed changes

src/Makefile Show resolved Hide resolved

Minor revision.

492b758

- Fixed format based on clang-format. - Re-added crccombine.o into Makefile. Signed-off-by: Kyle Kim <[email protected]>

kyle-yh-kim added a commit to kyle-yh-kim/valkey-doc that referenced this pull request Jun 25, 2024

Add CLUSTER SLOT-STATS document.

4bfdf02

- Valkey PR link; valkey-io/valkey#351. Signed-off-by: Kyle Kim <[email protected]>

kyle-yh-kim added a commit to kyle-yh-kim/valkey-doc that referenced this pull request Jun 25, 2024

Add CLUSTER SLOT-STATS document.

8ec5cbc

- Valkey PR link; valkey-io/valkey#351. Signed-off-by: Kyle Kim <[email protected]>

kyle-yh-kim mentioned this pull request Jun 25, 2024

Add CLUSTER SLOT-STATS document. valkey-io/valkey-doc#150

Open

kyle-yh-kim added a commit to kyle-yh-kim/valkey-doc that referenced this pull request Jun 25, 2024

Add CLUSTER SLOT-STATS document.

65d2006

- Valkey PR link; valkey-io/valkey#351. Signed-off-by: Kyle Kim <[email protected]>

Minor revision.

12730f0

- Updated RESP reply from map to array. - Renamed slotStatEntry to slotStatForSort. Signed-off-by: Kyle Kim <[email protected]>

kyle-yh-kim force-pushed the 20-slotstats branch from 2a8c171 to 12730f0 Compare June 26, 2024 02:59

madolson reviewed Jun 27, 2024

View reviewed changes

src/cluster_slot_stats.c Outdated Show resolved Hide resolved

Minor revision

6921c02

- Updated the RESP3 response from array of maps to array of arrays. Signed-off-by: Kyle Kim <[email protected]>

madolson reviewed Jun 27, 2024

View reviewed changes

Update src/commands/cluster-slot-stats.json

a9535f7

Signed-off-by: Madelyn Olson <[email protected]>

madolson reviewed Jun 27, 2024

View reviewed changes

src/commands/cluster-slot-stats.json Outdated Show resolved Hide resolved

madolson added 2 commits June 27, 2024 16:26

Update src/commands/cluster-slot-stats.json

8e1105f

Signed-off-by: Madelyn Olson <[email protected]>

Apply suggestions from code review

3cf86ab

Signed-off-by: Madelyn Olson <[email protected]>

madolson approved these changes Jun 27, 2024

View reviewed changes

madolson merged commit 1269532 into valkey-io:unstable Jun 27, 2024
20 checks passed

madolson reviewed Jun 28, 2024

View reviewed changes

src/cluster_slot_stats.c Show resolved Hide resolved

asafpamzn mentioned this pull request Jul 28, 2024

Valkey 8 support valkey-io/valkey-glide#2033

Closed

7 tasks

shohamazon mentioned this pull request Aug 19, 2024

Add Valkey8 support valkey-io/valkey-glide#2102

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce CLUSTER SLOT-STATS command (#20). #351

Introduce CLUSTER SLOT-STATS command (#20). #351

kyle-yh-kim commented Apr 23, 2024

madolson commented Apr 23, 2024 •

edited

Loading

kyle-yh-kim commented Apr 30, 2024

madolson commented May 7, 2024

codecov bot commented May 7, 2024 •

edited

Loading

madolson left a comment

madolson commented May 27, 2024

kyle-yh-kim commented Jun 18, 2024

madolson commented Jun 24, 2024

kyle-yh-kim commented Jun 25, 2024

Introduce CLUSTER SLOT-STATS command (#20). #351

Introduce CLUSTER SLOT-STATS command (#20). #351

Conversation

kyle-yh-kim commented Apr 23, 2024

madolson commented Apr 23, 2024 • edited Loading

kyle-yh-kim commented Apr 30, 2024

madolson commented May 7, 2024

codecov bot commented May 7, 2024 • edited Loading

Codecov Report

madolson left a comment

Choose a reason for hiding this comment

madolson commented May 27, 2024

kyle-yh-kim commented Jun 18, 2024

madolson commented Jun 24, 2024

kyle-yh-kim commented Jun 25, 2024

madolson commented Apr 23, 2024 •

edited

Loading

codecov bot commented May 7, 2024 •

edited

Loading