Minor revision.

- Updated RESP reply from map to array. - Renamed slotStatEntry to slotStatForSort. Signed-off-by: Kyle Kim <[email protected]>
valkey-io · Jun 26, 2024 · 12730f0 · 12730f0
1 parent 492b758
commit 12730f0
Show file tree

Hide file tree

Showing 4 changed files with 59 additions and 29 deletions.
diff --git a/src/cluster_slot_stats.c b/src/cluster_slot_stats.c
@@ -18,10 +18,11 @@ typedef enum {
  * CLUSTER SLOT-STATS command
  * -------------------------------------------------------------------------- */
 
+/* Struct used to temporarily hold slot statistics for sorting. */
 typedef struct {
     int slot;
     uint64_t stat;
-} slotStatEntry;
+} slotStatForSort;
 
 static int doesSlotBelongToMyShard(int slot) {
     clusterNode *myself = getMyClusterNode();
@@ -48,19 +49,19 @@ static uint64_t getSlotStat(int slot, int stat_type) {
     return slot_stat;
 }
 
-static int slotStatEntryAscCmp(const void *a, const void *b) {
-    slotStatEntry entry_a = *((slotStatEntry *)a);
-    slotStatEntry entry_b = *((slotStatEntry *)b);
+static int slotStatForSortAscCmp(const void *a, const void *b) {
+    slotStatForSort entry_a = *((slotStatForSort *)a);
+    slotStatForSort entry_b = *((slotStatForSort *)b);
     return entry_a.stat - entry_b.stat;
 }
 
-static int slotStatEntryDescCmp(const void *a, const void *b) {
-    slotStatEntry entry_a = *((slotStatEntry *)a);
-    slotStatEntry entry_b = *((slotStatEntry *)b);
+static int slotStatForSortDescCmp(const void *a, const void *b) {
+    slotStatForSort entry_a = *((slotStatForSort *)a);
+    slotStatForSort entry_b = *((slotStatForSort *)b);
     return entry_b.stat - entry_a.stat;
 }
 
-static void collectAndSortSlotStats(slotStatEntry slot_stats[], int order_by, int desc) {
+static void collectAndSortSlotStats(slotStatForSort slot_stats[], int order_by, int desc) {
     int i = 0;
 
     for (int slot = 0; slot < CLUSTER_SLOTS; slot++) {
@@ -70,36 +71,41 @@ static void collectAndSortSlotStats(slotStatEntry slot_stats[], int order_by, in
             i++;
         }
     }
-    qsort(slot_stats, i, sizeof(slotStatEntry), (desc) ? slotStatEntryDescCmp : slotStatEntryAscCmp);
+    qsort(slot_stats, i, sizeof(slotStatForSort), (desc) ? slotStatForSortDescCmp : slotStatForSortAscCmp);
 }
 
 static void addReplySlotStat(client *c, int slot) {
+    addReplyMapLen(c, 1); /* Map for (int) slot to (map) usage statistics. */
     addReplyLongLong(c, slot);
-    addReplyMapLen(c, 1);
+    addReplyMapLen(c, 1); /* Nested map representing slot usage statistics. */
     addReplyBulkCString(c, "key-count");
     addReplyLongLong(c, countKeysInSlot(slot));
 }
 
-static void addReplySlotStats(client *c, unsigned char *assigned_slots, int startslot, int endslot, int len) {
-    addReplyMapLen(c, len);
+/* Adds reply for the SLOTSRANGE variant.
+ * Response is ordered in ascending slot number. */
+static void addReplySlotsRange(client *c, unsigned char *assigned_slots, int startslot, int endslot, int len) {
+    addReplyArrayLen(c, len); /* Top level RESP reply format is defined as an array, due to ordering invariance. */
 
     for (int slot = startslot; slot <= endslot; slot++) {
         if (assigned_slots[slot]) addReplySlotStat(c, slot);
     }
 }
 
-static void addReplySortedSlotStats(client *c, slotStatEntry slot_stats[], long limit) {
+static void addReplySortedSlotStats(client *c, slotStatForSort slot_stats[], long limit) {
     int num_slots_assigned = getMyShardSlotCount();
     int len = min(limit, num_slots_assigned);
-    addReplyMapLen(c, len);
+    addReplyArrayLen(c, len); /* Top level RESP reply format is defined as an array, due to ordering invariance. */
 
     for (int i = 0; i < len; i++) {
         addReplySlotStat(c, slot_stats[i].slot);
     }
 }
 
-static void sortAndAddReplySlotStats(client *c, int order_by, long limit, int desc) {
-    slotStatEntry slot_stats[CLUSTER_SLOTS];
+/* Adds reply for the ORDERBY variant.
+ * Response is ordered based on the sort result. */
+static void addReplyOrderBy(client *c, int order_by, long limit, int desc) {
+    slotStatForSort slot_stats[CLUSTER_SLOTS];
     collectAndSortSlotStats(slot_stats, order_by, desc);
     addReplySortedSlotStats(c, slot_stats, limit);
 }
@@ -126,7 +132,7 @@ void clusterSlotStatsCommand(client *c) {
         unsigned char assigned_slots[CLUSTER_SLOTS] = {UNASSIGNED_SLOT};
         int len = 0;
         markSlotsAssignedToMyShard(assigned_slots, startslot, endslot, &len);
-        addReplySlotStats(c, assigned_slots, startslot, endslot, len);
+        addReplySlotsRange(c, assigned_slots, startslot, endslot, len);
 
     } else if (c->argc >= 4 && !strcasecmp(c->argv[2]->ptr, "orderby")) {
         /* CLUSTER SLOT-STATS ORDERBY metric [LIMIT limit] [ASC | DESC] */
@@ -165,7 +171,7 @@ void clusterSlotStatsCommand(client *c) {
             }
             i++;
         }
-        sortAndAddReplySlotStats(c, order_by, limit, desc);
+        addReplyOrderBy(c, order_by, limit, desc);
 
     } else {
         addReplySubcommandSyntaxError(c);

diff --git a/src/commands.def b/src/commands.def
@@ -1031,7 +1031,7 @@ struct COMMAND_STRUCT CLUSTER_Subcommands[] = {
 {MAKE_CMD("setslot","Binds a hash slot to a node.","O(1)","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_SETSLOT_History,1,CLUSTER_SETSLOT_Tips,0,clusterCommand,-4,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_STALE|CMD_MAY_REPLICATE,0,CLUSTER_SETSLOT_Keyspecs,0,NULL,3),.args=CLUSTER_SETSLOT_Args},
 {MAKE_CMD("shards","Returns the mapping of cluster slots to shards.","O(N) where N is the total number of cluster nodes","7.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_SHARDS_History,0,CLUSTER_SHARDS_Tips,1,clusterCommand,2,CMD_LOADING|CMD_STALE,0,CLUSTER_SHARDS_Keyspecs,0,NULL,0)},
 {MAKE_CMD("slaves","Lists the replica nodes of a primary node.","O(N) where N is the number of replicas.","3.0.0",CMD_DOC_DEPRECATED,"`CLUSTER REPLICAS`","5.0.0","cluster",COMMAND_GROUP_CLUSTER,CLUSTER_SLAVES_History,0,CLUSTER_SLAVES_Tips,1,clusterCommand,3,CMD_ADMIN|CMD_STALE,0,CLUSTER_SLAVES_Keyspecs,0,NULL,1),.args=CLUSTER_SLAVES_Args},
-{MAKE_CMD("slot-stats","Return array of slot usage statistics for slots assigned to the current node","O(N) where N is the total number of slots based on arguments. O(N*log(N)) with ORDERBY subcommand.","8.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_SLOT_STATS_History,0,CLUSTER_SLOT_STATS_Tips,2,clusterSlotStatsCommand,-4,CMD_STALE|CMD_LOADING,0,CLUSTER_SLOT_STATS_Keyspecs,0,NULL,1),.args=CLUSTER_SLOT_STATS_Args},
+{MAKE_CMD("slot-stats","Return an array of slot usage statistics for slots assigned to the current node.","O(N) where N is the total number of slots based on arguments. O(N*log(N)) with ORDERBY subcommand.","8.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_SLOT_STATS_History,0,CLUSTER_SLOT_STATS_Tips,2,clusterSlotStatsCommand,-4,CMD_STALE|CMD_LOADING,0,CLUSTER_SLOT_STATS_Keyspecs,0,NULL,1),.args=CLUSTER_SLOT_STATS_Args},
 {MAKE_CMD("slots","Returns the mapping of cluster slots to nodes.","O(N) where N is the total number of Cluster nodes","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_SLOTS_History,2,CLUSTER_SLOTS_Tips,1,clusterCommand,2,CMD_LOADING|CMD_STALE,0,CLUSTER_SLOTS_Keyspecs,0,NULL,0)},
 {0}
 };

diff --git a/src/commands/cluster-slot-stats.json b/src/commands/cluster-slot-stats.json
@@ -1,6 +1,6 @@
 {
     "SLOT-STATS": {
-        "summary": "Return array of slot usage statistics for slots assigned to the current node",
+        "summary": "Return an array of slot usage statistics for slots assigned to the current node.",
         "complexity": "O(N) where N is the total number of slots based on arguments. O(N*log(N)) with ORDERBY subcommand.",
         "group": "cluster",
         "since": "8.0.0",
@@ -16,10 +16,14 @@
             "REQUEST_POLICY:ALL_SHARDS"
         ],
         "reply_schema": {
-            "type": "object",
-            "description": "Map of slots and their respective usage statistics.",
-            "additionalProperties": {
-                "type": "string"
+            "type": "array",
+            "description": "Array of nested maps, where each map represents a slot and its respective usage statistics.",
+            "items": {
+                "type": "object",
+                "description": "Map of a slot and its respective usage statistics.",
+                "additionalProperties": {
+                    "type": "string"
+                }
             }
         },
         "arguments": [

diff --git a/tests/unit/cluster/slot-stats.tcl b/tests/unit/cluster/slot-stats.tcl
@@ -4,6 +4,19 @@
 # Helper functions for CLUSTER SLOT-STATS test cases.
 # -----------------------------------------------------------------------------
 
+# Converts array RESP response into a dict.
+# This is useful for many test cases, where unnecessary nesting is removed.
+proc convert_array_into_dict {slot_stats} {
+    set res [dict create]
+    foreach slot_stat $slot_stats {
+        # slot_stat is a map of (int) slot to (map) usage statistics.
+        dict for {slot stat} $slot_stat {
+            dict set res $slot $stat
+        }
+    }
+    return $res
+}
+
 proc initialize_expected_slots_dict {} {
     set expected_slots [dict create]
     for {set i 0} {$i < 16384} {incr i 1} {
@@ -22,12 +35,14 @@ proc initialize_expected_slots_dict_with_range {start_slot end_slot} {
 }
 
 proc assert_empty_slot_stats {slot_stats} {
+    set slot_stats [convert_array_into_dict $slot_stats]
     dict for {slot stats} $slot_stats {
         assert {[dict get $stats key-count] == 0}
     }
 }
 
 proc assert_empty_slot_stats_with_exception {slot_stats exception_slots} {
+    set slot_stats [convert_array_into_dict $slot_stats]
     dict for {slot stats} $slot_stats {
         if {[dict exists $exception_slots $slot]} {
             set expected_key_count [dict get $exception_slots $slot]
@@ -45,6 +60,7 @@ proc assert_all_slots_have_been_seen {expected_slots} {
 }
 
 proc assert_slot_visibility {slot_stats expected_slots} {
+    set slot_stats [convert_array_into_dict $slot_stats]
     dict for {slot _} $slot_stats {
         assert {[dict exists $expected_slots $slot]}
         dict set expected_slots $slot 1
@@ -54,6 +70,7 @@ proc assert_slot_visibility {slot_stats expected_slots} {
 }
 
 proc assert_slot_stats_key_count {slot_stats expected_slots_key_count} {
+    set slot_stats [convert_array_into_dict $slot_stats]
     dict for {slot stats} $slot_stats {
         if {[dict exists $expected_slots_key_count $slot]} {
             set key_count [dict get $stats key-count]
@@ -64,8 +81,11 @@ proc assert_slot_stats_key_count {slot_stats expected_slots_key_count} {
 }
 
 proc assert_slot_stats_monotonic_order {slot_stats orderby is_desc} {
+    # For Tcl dict, the order of iteration is the order in which the keys were inserted into the dictionary
+    # Thus, the response ordering is preserved upon calling 'convert_array_into_dict()'.
+    # Source: https://www.tcl.tk/man/tcl8.6.11/TclCmd/dict.htm
+    set slot_stats [convert_array_into_dict $slot_stats]
     set prev_metric -1
-
     dict for {_ stats} $slot_stats {
         set curr_metric [dict get $stats $orderby]
         if {$prev_metric != -1} {
@@ -217,8 +237,8 @@ start_cluster 1 0 {tags {external:skip cluster}} {
         set limit 5
         set slot_stats_desc [R 0 CLUSTER SLOT-STATS ORDERBY key-count LIMIT $limit DESC]
         set slot_stats_asc [R 0 CLUSTER SLOT-STATS ORDERBY key-count LIMIT $limit ASC]
-        set slot_stats_desc_length [dict size $slot_stats_desc]
-        set slot_stats_asc_length [dict size $slot_stats_asc]
+        set slot_stats_desc_length [llength $slot_stats_desc]
+        set slot_stats_asc_length [llength $slot_stats_asc]
         assert {$limit == $slot_stats_desc_length && $limit == $slot_stats_asc_length}
 
         set expected_slots [dict create 0 0 1 0 2 0 3 0 4 0]
@@ -236,8 +256,8 @@ start_cluster 1 0 {tags {external:skip cluster}} {
         set limit 5
         set slot_stats_desc [R 0 CLUSTER SLOT-STATS ORDERBY key-count LIMIT $limit DESC]
         set slot_stats_asc [R 0 CLUSTER SLOT-STATS ORDERBY key-count LIMIT $limit ASC]
-        set slot_stats_desc_length [dict size $slot_stats_desc]
-        set slot_stats_asc_length [dict size $slot_stats_asc]
+        set slot_stats_desc_length [llength $slot_stats_desc]
+        set slot_stats_asc_length [llength $slot_stats_asc]
         set expected_response_length [expr min($num_assigned_slots, $limit)]
         assert {$expected_response_length == $slot_stats_desc_length && $expected_response_length == $slot_stats_asc_length}