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.

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

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

Merged
merged 11 commits into from
Jun 27, 2024

Conversation

kyle-yh-kim
Copy link
Contributor

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
Copy link
Member

madolson commented Apr 23, 2024

Run:

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

To fix the DCO complaint.

@madolson madolson added the major-decision-pending Major decision pending by TSC team label Apr 23, 2024
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 <kimkyle@amazon.com>
@kyle-yh-kim
Copy link
Contributor Author

Thanks for sharing. Two changes added;

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

@madolson
Copy link
Member

madolson commented May 7, 2024

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

Copy link

codecov bot commented May 7, 2024

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

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 remember most of this, and it still mostly LGTM.

src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
@madolson
Copy link
Member

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 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
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
src/cluster_slots.c Outdated Show resolved Hide resolved
- Renamed cluster_slots.c to cluster_slot_stats.c
- Renamed function signature and variables.
- Removed 0 argument support.

Signed-off-by: Kyle Kim <kimkyle@amazon.com>
@kyle-yh-kim
Copy link
Contributor Author

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.

@madolson 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
- Update reply_schema.

Signed-off-by: Kyle Kim <kimkyle@amazon.com>
@madolson 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
Copy link
Member

@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 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
Signed-off-by: Kyle Kim <kimkyle@amazon.com>
- Fixed origin/unstable merge conflicts.
- Updated formatting.
- Updated cluster-slot-stats.json.

Signed-off-by: Kyle Kim <kimkyle@amazon.com>
src/Makefile Show resolved Hide resolved
- Fixed format based on clang-format.
- Re-added crccombine.o into Makefile.

Signed-off-by: Kyle Kim <kimkyle@amazon.com>
kyle-yh-kim added a commit to kyle-yh-kim/valkey-doc that referenced this pull request Jun 25, 2024
- Valkey PR link; valkey-io/valkey#351.

Signed-off-by: Kyle Kim <kimkyle@amazon.com>
kyle-yh-kim added a commit to kyle-yh-kim/valkey-doc that referenced this pull request Jun 25, 2024
- Valkey PR link; valkey-io/valkey#351.

Signed-off-by: Kyle Kim <kimkyle@amazon.com>
@kyle-yh-kim
Copy link
Contributor Author

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

kyle-yh-kim added a commit to kyle-yh-kim/valkey-doc that referenced this pull request Jun 25, 2024
- Valkey PR link; valkey-io/valkey#351.

Signed-off-by: Kyle Kim <kimkyle@amazon.com>
- Updated RESP reply from map to array.
- Renamed slotStatEntry to slotStatForSort.

Signed-off-by: Kyle Kim <kimkyle@amazon.com>
src/cluster_slot_stats.c Outdated Show resolved Hide resolved
- Updated the RESP3 response from array of maps to array of arrays.

Signed-off-by: Kyle Kim <kimkyle@amazon.com>
src/cluster_slot_stats.c Outdated Show resolved Hide resolved
src/cluster_slot_stats.c Outdated Show resolved Hide resolved
src/cluster_slot_stats.c 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
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
@madolson madolson merged commit 1269532 into valkey-io:unstable Jun 27, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants