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 <kimkyle@amazon.com>

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 89.32039% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 68.42%. Comparing base (87a5bfc) to head (e17019b).
Report is 46 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #351      +/-   ##
============================================
+ Coverage     68.40%   68.42%   +0.02%     
============================================
  Files           108      109       +1     
  Lines         61562    61662     +100     
============================================
+ Hits          42111    42195      +84     
- Misses        19451    19467      +16

Files	Coverage Δ
src/commands.def	`100.00% <ø> (ø)`
src/cluster_slots.c	`89.32% <89.32%> (ø)`

... and 15 files with indirect coverage changes

madolson

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

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

src/cluster_slots.c

+ * Copyright (c) 2024, Kyle Kim <kimkyle at amazon dot com>, Amazon Web Services.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ *   * Redistributions of source code must retain the above copyright notice,
+ *     this list of conditions and the following disclaimer.
+ *   * Redistributions in binary form must reproduce the above copyright
+ *     notice, this list of conditions and the following disclaimer in the
+ *     documentation and/or other materials provided with the distribution.
+ *   * Neither the name of Valkey nor the names of its contributors may be used
+ *     to endorse or promote products derived from this software without
+ *     specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */


Suggested change

* Copyright (c) 2024, Kyle Kim <kimkyle at amazon dot com>, Amazon Web Services.

* All rights reserved.

*

* Redistribution and use in source and binary forms, with or without

* modification, are permitted provided that the following conditions are met:

*

* * Redistributions of source code must retain the above copyright notice,

* this list of conditions and the following disclaimer.

* * Redistributions in binary form must reproduce the above copyright

* notice, this list of conditions and the following disclaimer in the

* documentation and/or other materials provided with the distribution.

* * Neither the name of Valkey nor the names of its contributors may be used

* to endorse or promote products derived from this software without

* specific prior written permission.

*

* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"

* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE

* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE

* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE

* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR

* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF

* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS

* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN

* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)

* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE

* POSSIBILITY OF SUCH DAMAGE.

*/

*

* Copyright Valkey contributors.

* All rights reserved.

* SPDX-License-Identifier: BSD 3-Clause

*/

We live in the future where you get to write less stuff.

madolson · 2024-05-07T23:33:55Z

src/cluster_slots.c

+        checkSlotAssignment(slots, startslot, endslot);
+        addReplySlotStats(c, slots);
+    } else if (!strcasecmp(c->argv[2]->ptr,"orderby") && c->argc >= 4) {
+        /* CLUSTER SLOT-STATS ORDERBY column [LIMIT limit] [ASC | DESC] */


Suggested change

/* CLUSTER SLOT-STATS ORDERBY column [LIMIT limit] [ASC | DESC] */

/* CLUSTER SLOT-STATS ORDERBY metric [LIMIT limit] [ASC | DESC] */

Did we really use column before? Seems like the wrong wording for sure.

madolson · 2024-05-07T23:35:58Z

src/cluster_slots.c

+#include "cluster.h"
+#include "cluster_legacy.h"


What do you need from cluster_legacy.h? We have some vague notion that we want to better abstract away cluster_legacy unless it's dealing with the clusterbus and/or low-level topology.

madolson · 2024-05-07T23:38:15Z

src/cluster_slots.c

+    /* No further arguments. */
+    if (c->argc == 2) {
+        /* CLUSTER SLOT-STATS */
+        markAssignedSlots(slots);
+        addReplySlotStats(c, slots);
+        return;
+    }


Have we ever called this operationally? I feel like we always call it filtered. Maybe we should move away from a foot gun and encourage people to limit the amount of data they pull by deleting the 2 arg form? Not a strong opinion, just something I thought about since I'm taking a fresh look.

madolson · 2024-05-07T23:39:43Z

src/cluster_slots.c

+#include "cluster_legacy.h"
+
+#define DEFAULT_SLOT -1
+#define DEFAULT_STAT 0


What is this used for?

madolson · 2024-05-07T23:40:12Z

src/cluster_slots.c

+#include "cluster.h"
+#include "cluster_legacy.h"
+
+#define DEFAULT_SLOT -1


What is this used for? Either remove or document it.

madolson · 2024-05-07T23:42:38Z

src/cluster_slots.c

@@ -0,0 +1,217 @@
+/* Cluster slots APIs and commands - to retrieve, update and process slot level data 


I remember we named this because we planned to break up the APIs, but we ended up naming it weirdly (what is a "legacy cluster" when there is only one cluster) but it doesn't make a lot of sense to revert it now. I don't mind this in its own file, but maybe we call it cluster_slot_stats instead?

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

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

Open

madolson added the major-decision-pending Needs decision by core 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

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

kyle-yh-kim commented Apr 30, 2024

madolson commented May 7, 2024

codecov bot commented May 7, 2024

madolson left a comment

madolson May 7, 2024

madolson May 7, 2024

madolson May 7, 2024

madolson May 7, 2024

madolson May 7, 2024

madolson May 7, 2024

madolson May 7, 2024

	/* CLUSTER SLOT-STATS ORDERBY column [LIMIT limit] [ASC \| DESC] */
	/* CLUSTER SLOT-STATS ORDERBY metric [LIMIT limit] [ASC \| DESC] */

		@@ -0,0 +1,217 @@
		/* Cluster slots APIs and commands - to retrieve, update and process slot level data

		#include "cluster.h"
		#include "cluster_legacy.h"

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

Are you sure you want to change the base?

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

Conversation

kyle-yh-kim commented Apr 23, 2024

madolson commented Apr 23, 2024 • edited

kyle-yh-kim commented Apr 30, 2024

madolson commented May 7, 2024

codecov bot commented May 7, 2024

Codecov Report

madolson left a comment

Choose a reason for hiding this comment

madolson May 7, 2024

Choose a reason for hiding this comment

madolson May 7, 2024

Choose a reason for hiding this comment

madolson May 7, 2024

Choose a reason for hiding this comment

madolson May 7, 2024

Choose a reason for hiding this comment

madolson May 7, 2024

Choose a reason for hiding this comment

madolson May 7, 2024

Choose a reason for hiding this comment

madolson May 7, 2024

Choose a reason for hiding this comment

madolson commented Apr 23, 2024 •

edited