From e17019bb8338197ea8e6eb682b441223f8900bed Mon Sep 17 00:00:00 2001 From: Kyle Kim Date: Mon, 22 Apr 2024 05:25:42 +0000 Subject: [PATCH 01/10] Introduce CLUSTER SLOT-STATS command (#20). 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 --- src/Makefile | 2 +- src/cluster.h | 2 + src/cluster_slots.c | 217 +++++++++++++++++++ src/commands.def | 51 +++++ src/commands/cluster-slot-stats.json | 79 +++++++ src/server.h | 1 + tests/unit/cluster/slot-stats.tcl | 304 +++++++++++++++++++++++++++ 7 files changed, 655 insertions(+), 1 deletion(-) create mode 100644 src/cluster_slots.c create mode 100644 src/commands/cluster-slot-stats.json create mode 100644 tests/unit/cluster/slot-stats.tcl diff --git a/src/Makefile b/src/Makefile index 47b961862a..b55be582e2 100644 --- a/src/Makefile +++ b/src/Makefile @@ -383,7 +383,7 @@ endif ENGINE_NAME=valkey SERVER_NAME=$(ENGINE_NAME)-server$(PROG_SUFFIX) ENGINE_SENTINEL_NAME=$(ENGINE_NAME)-sentinel$(PROG_SUFFIX) -ENGINE_SERVER_OBJ=threads_mngr.o adlist.o quicklist.o ae.o anet.o dict.o kvstore.o server.o sds.o zmalloc.o lzf_c.o lzf_d.o pqsort.o zipmap.o sha1.o ziplist.o release.o networking.o util.o object.o db.o replication.o rdb.o t_string.o t_list.o t_set.o t_zset.o t_hash.o config.o aof.o pubsub.o multi.o debug.o sort.o intset.o syncio.o cluster.o cluster_legacy.o crc16.o endianconv.o slowlog.o eval.o bio.o rio.o rand.o memtest.o syscheck.o crcspeed.o crc64.o bitops.o sentinel.o notify.o setproctitle.o blocked.o hyperloglog.o latency.o sparkline.o valkey-check-rdb.o valkey-check-aof.o geo.o lazyfree.o module.o evict.o expire.o geohash.o geohash_helper.o childinfo.o defrag.o siphash.o rax.o t_stream.o listpack.o localtime.o lolwut.o lolwut5.o lolwut6.o acl.o tracking.o socket.o tls.o sha256.o timeout.o setcpuaffinity.o monotonic.o mt19937-64.o resp_parser.o call_reply.o script_lua.o script.o functions.o function_lua.o commands.o strl.o connection.o unix.o logreqres.o +ENGINE_SERVER_OBJ=threads_mngr.o adlist.o quicklist.o ae.o anet.o dict.o kvstore.o server.o sds.o zmalloc.o lzf_c.o lzf_d.o pqsort.o zipmap.o sha1.o ziplist.o release.o networking.o util.o object.o db.o replication.o rdb.o t_string.o t_list.o t_set.o t_zset.o t_hash.o config.o aof.o pubsub.o multi.o debug.o sort.o intset.o syncio.o cluster.o cluster_legacy.o cluster_slots.o crc16.o endianconv.o slowlog.o eval.o bio.o rio.o rand.o memtest.o syscheck.o crcspeed.o crc64.o bitops.o sentinel.o notify.o setproctitle.o blocked.o hyperloglog.o latency.o sparkline.o valkey-check-rdb.o valkey-check-aof.o geo.o lazyfree.o module.o evict.o expire.o geohash.o geohash_helper.o childinfo.o defrag.o siphash.o rax.o t_stream.o listpack.o localtime.o lolwut.o lolwut5.o lolwut6.o acl.o tracking.o socket.o tls.o sha256.o timeout.o setcpuaffinity.o monotonic.o mt19937-64.o resp_parser.o call_reply.o script_lua.o script.o functions.o function_lua.o commands.o strl.o connection.o unix.o logreqres.o ENGINE_CLI_NAME=$(ENGINE_NAME)-cli$(PROG_SUFFIX) ENGINE_CLI_OBJ=anet.o adlist.o dict.o valkey-cli.o zmalloc.o release.o ae.o serverassert.o crcspeed.o crc64.o siphash.o crc16.o monotonic.o cli_common.o mt19937-64.o strl.o cli_commands.o ENGINE_BENCHMARK_NAME=$(ENGINE_NAME)-benchmark$(PROG_SUFFIX) diff --git a/src/cluster.h b/src/cluster.h index a7211615dd..c9155f33c3 100644 --- a/src/cluster.h +++ b/src/cluster.h @@ -103,6 +103,8 @@ char *clusterNodeHostname(clusterNode *node); const char *clusterNodePreferredEndpoint(clusterNode *n); long long clusterNodeReplOffset(clusterNode *node); clusterNode *clusterLookupNode(const char *name, int length); +unsigned int countKeysInSlot(unsigned int hashslot); +int getSlotOrReply(client *c, robj *o); /* functions with shared implementations */ clusterNode *getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, int argc, int *hashslot, int *ask); diff --git a/src/cluster_slots.c b/src/cluster_slots.c new file mode 100644 index 0000000000..7330c008d8 --- /dev/null +++ b/src/cluster_slots.c @@ -0,0 +1,217 @@ +/* Cluster slots APIs and commands - to retrieve, update and process slot level data + * in association with Valkey cluster. + * + * Copyright (c) 2024, Kyle Kim , 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. + */ + +#include "server.h" +#include "cluster.h" +#include "cluster_legacy.h" + +#define DEFAULT_SLOT -1 +#define DEFAULT_STAT 0 +#define UNASSIGNED_SLOT 0 +#define ORDER_BY_KEY_COUNT 1 +#define ORDER_BY_INVALID -1 + +/* ----------------------------------------------------------------------------- + * CLUSTER SLOT-STATS command + * -------------------------------------------------------------------------- */ + +typedef struct sortedSlotStatEntry { + int slot; + uint64_t stat; +} sortedSlotStatEntry; + +static int doesSlotBelongToMyShard(int slot) { + clusterNode *n = clusterNodeGetMaster(server.cluster->myself); + return server.cluster->slots[slot] == n; +} + +static void markAssignedSlots(unsigned char *slots) { + for (int slot = 0; slot < CLUSTER_SLOTS; slot++) { + if (doesSlotBelongToMyShard(slot)) slots[slot]++; + } +} + +static int countAssignedSlotsFromSlotsArray(unsigned char *slots) { + int count = 0; + for (int slot = 0; slot < CLUSTER_SLOTS; slot++) { + if (slots[slot]) count++; + } + return count; +} + +static void checkSlotAssignment(unsigned char *slots, int start_slot, int end_slot) { + for (int slot = start_slot; slot <= end_slot; slot++) { + if (doesSlotBelongToMyShard(slot)) { + slots[slot]++; + } + } +} + +static uint64_t getSingleSlotStat(int slot, int order_by) { + serverAssert(order_by != ORDER_BY_INVALID); + uint64_t singleSlotStat = 0; + if (order_by == ORDER_BY_KEY_COUNT) { + singleSlotStat = countKeysInSlot(slot); + } + return singleSlotStat; +} + +static int slotStatEntryAscCmp(const void *a, const void *b) { + sortedSlotStatEntry entry_a = *((sortedSlotStatEntry *) a); + sortedSlotStatEntry entry_b = *((sortedSlotStatEntry *) b); + return entry_a.stat - entry_b.stat; +} + +static int slotStatEntryDescCmp(const void *a, const void *b) { + sortedSlotStatEntry entry_a = *((sortedSlotStatEntry *) a); + sortedSlotStatEntry entry_b = *((sortedSlotStatEntry *) b); + return entry_b.stat - entry_a.stat; +} + +static void sortSlotStats(sortedSlotStatEntry sorted[], int order_by, int desc) { + int i = 0; + + for (int slot = 0; slot < CLUSTER_SLOTS; slot++) { + if (doesSlotBelongToMyShard(slot)) { + sorted[i].slot = slot; + sorted[i].stat = getSingleSlotStat(slot, order_by); + i++; + } + } + qsort(sorted, i, sizeof(sortedSlotStatEntry), (desc) ? slotStatEntryDescCmp : slotStatEntryAscCmp); +} + +static void addReplySingleSlotStat(client *c, int slot) { + addReplyLongLong(c, slot); + addReplyMapLen(c, 1); + addReplyBulkCString(c, "key-count"); + addReplyLongLong(c, countKeysInSlot(slot)); +} + +static void addReplySlotStats(client *c, unsigned char *slots) { + int num_slots_assigned = countAssignedSlotsFromSlotsArray(slots); + addReplyMapLen(c, num_slots_assigned); + + for (int slot = 0; slot < CLUSTER_SLOTS; slot++) { + if (slots[slot]) addReplySingleSlotStat(c, slot); + } +} + +static void addReplySortedSlotStats(client *c, sortedSlotStatEntry sorted[], long limit) { + int num_slots_assigned = getMyShardSlotCount(); + int len = min(limit, num_slots_assigned); + addReplyMapLen(c, len); + + for (int i = 0; i < len; i++) { + addReplySingleSlotStat(c, sorted[i].slot); + } +} + +static void sortAndAddReplySlotStats(client *c, int order_by, long limit, int desc) { + sortedSlotStatEntry sorted[CLUSTER_SLOTS]; + sortSlotStats(sorted, order_by, desc); + addReplySortedSlotStats(c, sorted, limit); +} + +void clusterSlotStatsCommand(client *c) { + if (server.cluster_enabled == 0) { + addReplyError(c,"This instance has cluster support disabled"); + return; + } + + /* Initialize slot assignment array. */ + unsigned char slots[CLUSTER_SLOTS]= {UNASSIGNED_SLOT}; + + /* No further arguments. */ + if (c->argc == 2) { + /* CLUSTER SLOT-STATS */ + markAssignedSlots(slots); + addReplySlotStats(c, slots); + return; + } + + /* Parse additional arguments. */ + if (!strcasecmp(c->argv[2]->ptr,"slotsrange") && c->argc == 5) { + /* CLUSTER SLOT-STATS SLOTSRANGE start-slot end-slot */ + int startslot, endslot; + if ((startslot = getSlotOrReply(c,c->argv[3])) == C_ERR || + (endslot = getSlotOrReply(c,c->argv[4])) == C_ERR) { + return; + } + if (startslot > endslot) { + addReplyErrorFormat(c,"start slot number %d is greater than end slot number %d", startslot, endslot); + return; + } + 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] */ + int desc = 1, order_by = ORDER_BY_INVALID; + if (!strcasecmp(c->argv[3]->ptr, "key-count")) { + order_by = ORDER_BY_KEY_COUNT; + } else { + addReplyError(c, "unrecognized sort column for ORDER BY. The supported columns are: key-count."); + return; + } + int i = 4; /* Next argument index, following ORDERBY */ + int limit_counter = 0, asc_desc_counter = 0; + long limit; + while(i < c->argc) { + int moreargs = c->argc > i+1; + if (!strcasecmp(c->argv[i]->ptr,"limit") && moreargs) { + if (getRangeLongFromObjectOrReply( + c, c->argv[i+1], 1, CLUSTER_SLOTS, &limit, + "limit has to lie in between 1 and 16384 (maximum number of slots)") != C_OK) + return; + i++; + limit_counter++; + } else if (!strcasecmp(c->argv[i]->ptr,"asc")) { + desc = 0; + asc_desc_counter++; + } else if (!strcasecmp(c->argv[i]->ptr,"desc")) { + desc = 1; + asc_desc_counter++; + } else { + addReplyErrorObject(c,shared.syntaxerr); + return; + } + + if (limit_counter > 1 || asc_desc_counter > 1) { + addReplyError(c, "you cannot provide multiple filters of the same type."); + return; + } + i++; + } + sortAndAddReplySlotStats(c, order_by, limit, desc); + } else { + addReplySubcommandSyntaxError(c); + } +} diff --git a/src/commands.def b/src/commands.def index bd6ed38153..9208f26553 100644 --- a/src/commands.def +++ b/src/commands.def @@ -921,6 +921,56 @@ struct COMMAND_ARG CLUSTER_SLAVES_Args[] = { {MAKE_ARG("node-id",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, }; +/********** CLUSTER SLOT_STATS ********************/ + +#ifndef SKIP_CMD_HISTORY_TABLE +/* CLUSTER SLOT_STATS history */ +#define CLUSTER_SLOT_STATS_History NULL +#endif + +#ifndef SKIP_CMD_TIPS_TABLE +/* CLUSTER SLOT_STATS tips */ +const char *CLUSTER_SLOT_STATS_Tips[] = { +"nondeterministic_output", +"all_shards", +}; +#endif + +#ifndef SKIP_CMD_KEY_SPECS_TABLE +/* CLUSTER SLOT_STATS key specs */ +#define CLUSTER_SLOT_STATS_Keyspecs NULL +#endif + +/* CLUSTER SLOT_STATS filter slotsrange argument table */ +struct COMMAND_ARG CLUSTER_SLOT_STATS_filter_slotsrange_Subargs[] = { +{MAKE_ARG("start-slot",ARG_TYPE_INTEGER,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("end-slot",ARG_TYPE_INTEGER,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, +}; + +/* CLUSTER SLOT_STATS filter orderby order argument table */ +struct COMMAND_ARG CLUSTER_SLOT_STATS_filter_orderby_order_Subargs[] = { +{MAKE_ARG("asc",ARG_TYPE_PURE_TOKEN,-1,"ASC",NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("desc",ARG_TYPE_PURE_TOKEN,-1,"DESC",NULL,NULL,CMD_ARG_NONE,0,NULL)}, +}; + +/* CLUSTER SLOT_STATS filter orderby argument table */ +struct COMMAND_ARG CLUSTER_SLOT_STATS_filter_orderby_Subargs[] = { +{MAKE_ARG("column",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("limit",ARG_TYPE_INTEGER,-1,"LIMIT",NULL,NULL,CMD_ARG_OPTIONAL,0,NULL)}, +{MAKE_ARG("order",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,2,NULL),.subargs=CLUSTER_SLOT_STATS_filter_orderby_order_Subargs}, +}; + +/* CLUSTER SLOT_STATS filter argument table */ +struct COMMAND_ARG CLUSTER_SLOT_STATS_filter_Subargs[] = { +{MAKE_ARG("slotsrange",ARG_TYPE_BLOCK,-1,"SLOTSRANGE",NULL,NULL,CMD_ARG_OPTIONAL,2,NULL),.subargs=CLUSTER_SLOT_STATS_filter_slotsrange_Subargs}, +{MAKE_ARG("orderby",ARG_TYPE_BLOCK,-1,"ORDERBY",NULL,NULL,CMD_ARG_OPTIONAL,3,NULL),.subargs=CLUSTER_SLOT_STATS_filter_orderby_Subargs}, +}; + +/* CLUSTER SLOT_STATS argument table */ +struct COMMAND_ARG CLUSTER_SLOT_STATS_Args[] = { +{MAKE_ARG("filter",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,2,NULL),.subargs=CLUSTER_SLOT_STATS_filter_Subargs}, +}; + /********** CLUSTER SLOTS ********************/ #ifndef SKIP_CMD_HISTORY_TABLE @@ -972,6 +1022,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,0,CLUSTER_SETSLOT_Tips,0,clusterCommand,-4,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_STALE,0,CLUSTER_SETSLOT_Keyspecs,0,NULL,2),.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 master 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.","7.2.6",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_SLOT_STATS_History,0,CLUSTER_SLOT_STATS_Tips,2,clusterSlotStatsCommand,-2,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_DEPRECATED,"`CLUSTER SHARDS`","7.0.0","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 new file mode 100644 index 0000000000..2f1aebb67e --- /dev/null +++ b/src/commands/cluster-slot-stats.json @@ -0,0 +1,79 @@ +{ + "SLOT-STATS": { + "summary": "Return 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": "7.2.6", + "arity": -2, + "container": "CLUSTER", + "function": "clusterSlotStatsCommand", + "command_flags": [ + "STALE", + "LOADING" + ], + "command_tips": [ + "NONDETERMINISTIC_OUTPUT", + "ALL_SHARDS" + ], + "arguments": [ + { + "name": "filter", + "type": "oneof", + "optional": true, + "arguments": [ + { + "token": "SLOTSRANGE", + "name": "slotsrange", + "type": "block", + "optional": true, + "arguments": [ + { + "name": "start-slot", + "type": "integer" + }, + { + "name": "end-slot", + "type": "integer" + } + ] + }, + { + "token": "ORDERBY", + "name": "orderby", + "type": "block", + "optional": true, + "arguments": [ + { + "name": "column", + "type": "string" + }, + { + "token": "LIMIT", + "name": "limit", + "type": "integer", + "optional": true + }, + { + "name": "order", + "type": "oneof", + "optional": true, + "arguments": [ + { + "name": "asc", + "type": "pure-token", + "token": "ASC" + }, + { + "name": "desc", + "type": "pure-token", + "token": "DESC" + } + ] + } + ] + } + ] + } + ] + } +} \ No newline at end of file diff --git a/src/server.h b/src/server.h index 7f9a5c5cc4..b1cbdb2afb 100644 --- a/src/server.h +++ b/src/server.h @@ -3662,6 +3662,7 @@ void sunsubscribeCommand(client *c); void watchCommand(client *c); void unwatchCommand(client *c); void clusterCommand(client *c); +void clusterSlotStatsCommand(client *c); void restoreCommand(client *c); void migrateCommand(client *c); void askingCommand(client *c); diff --git a/tests/unit/cluster/slot-stats.tcl b/tests/unit/cluster/slot-stats.tcl new file mode 100644 index 0000000000..524efed885 --- /dev/null +++ b/tests/unit/cluster/slot-stats.tcl @@ -0,0 +1,304 @@ +# Integration tests for CLUSTER SLOT-STATS command. + +# ----------------------------------------------------------------------------- +# Helper functions for CLUSTER SLOT-STATS test cases. +# ----------------------------------------------------------------------------- + +proc initialize_expected_slots_dict {} { + set expected_slots [dict create] + for {set i 0} {$i < 16384} {incr i 1} { + dict set expected_slots $i 0 + } + return $expected_slots +} + +proc initialize_expected_slots_dict_with_range {start_slot end_slot} { + assert {$start_slot <= $end_slot} + set expected_slots [dict create] + for {set i $start_slot} {$i <= $end_slot} {incr i 1} { + dict set expected_slots $i 0 + } + return $expected_slots +} + +proc assert_empty_slot_stats {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} { + dict for {slot stats} $slot_stats { + if {[dict exists $exception_slots $slot]} { + set expected_key_count [dict get $exception_slots $slot] + assert {[dict get $stats key-count] == $expected_key_count} + } else { + assert {[dict get $stats key-count] == 0} + } + } +} + +proc assert_all_slots_have_been_seen {expected_slots} { + dict for {k v} $expected_slots { + assert {$v == 1} + } +} + +proc assert_slot_visibility {slot_stats expected_slots} { + dict for {slot _} $slot_stats { + assert {[dict exists $expected_slots $slot]} + dict set expected_slots $slot 1 + } + + assert_all_slots_have_been_seen $expected_slots +} + +proc assert_slot_stats_key_count {slot_stats expected_slots_key_count} { + dict for {slot stats} $slot_stats { + if {[dict exists $expected_slots_key_count $slot]} { + set key_count [dict get $stats key-count] + set key_count_expected [dict get $expected_slots_key_count $slot] + assert {$key_count == $key_count_expected} + } + } +} + +proc assert_slot_stats_monotonic_order {slot_stats orderby is_desc} { + set prev_metric -1 + + dict for {_ stats} $slot_stats { + set curr_metric [dict get $stats $orderby] + if {$prev_metric != -1} { + if {$is_desc == 1} { + assert {$prev_metric >= $curr_metric} + } else { + assert {$prev_metric <= $curr_metric} + } + } + set prev_metric $curr_metric + } +} + +proc assert_slot_stats_monotonic_descent {slot_stats orderby} { + assert_slot_stats_monotonic_order $slot_stats $orderby 1 +} + +proc assert_slot_stats_monotonic_ascent {slot_stats orderby} { + assert_slot_stats_monotonic_order $slot_stats $orderby 0 +} + +proc wait_for_replica_key_exists {key key_count} { + wait_for_condition 1000 50 { + [R 1 exists $key] eq "$key_count" + } else { + fail "Test key was not replicated" + } +} + +# ----------------------------------------------------------------------------- +# Test cases for CLUSTER SLOT-STATS correctness, without additional arguments. +# ----------------------------------------------------------------------------- + +start_cluster 1 0 {tags {external:skip cluster}} { + + # Define shared variables. + set key "FOO" + set key_slot [R 0 cluster keyslot $key] + set expected_slots_to_key_count [dict create $key_slot 1] + + test "CLUSTER SLOT-STATS contains default value upon valkey-server startup" { + set slot_stats [R 0 CLUSTER SLOT-STATS] + assert_empty_slot_stats $slot_stats + } + + test "CLUSTER SLOT-STATS contains correct metrics upon key introduction" { + R 0 SET $key TEST + set slot_stats [R 0 CLUSTER SLOT-STATS] + assert_empty_slot_stats_with_exception $slot_stats $expected_slots_to_key_count + } + + test "CLUSTER SLOT-STATS contains correct metrics upon key mutation" { + R 0 SET $key NEW_VALUE + set slot_stats [R 0 CLUSTER SLOT-STATS] + assert_empty_slot_stats_with_exception $slot_stats $expected_slots_to_key_count + } + + test "CLUSTER SLOT-STATS contains correct metrics upon key deletion" { + R 0 DEL $key + set slot_stats [R 0 CLUSTER SLOT-STATS] + assert_empty_slot_stats $slot_stats + } + + test "CLUSTER SLOT-STATS slot visibility based on slot ownership changes" { + R 0 CONFIG SET cluster-require-full-coverage no + + R 0 CLUSTER DELSLOTS $key_slot + set expected_slots [initialize_expected_slots_dict] + dict unset expected_slots $key_slot + set slot_stats [R 0 CLUSTER SLOT-STATS] + assert {[dict size $expected_slots] == 16383} + assert_slot_visibility $slot_stats $expected_slots + + R 0 CLUSTER ADDSLOTS $key_slot + set expected_slots [initialize_expected_slots_dict] + set slot_stats [R 0 CLUSTER SLOT-STATS] + assert {[dict size $expected_slots] == 16384} + assert_slot_visibility $slot_stats $expected_slots + } +} + +# ----------------------------------------------------------------------------- +# Test cases for CLUSTER SLOT-STATS SLOTSRANGE sub-argument. +# ----------------------------------------------------------------------------- + +start_cluster 1 0 {tags {external:skip cluster}} { + + test "CLUSTER SLOT-STATS SLOTSRANGE all slots present" { + set start_slot 100 + set end_slot 102 + set expected_slots [initialize_expected_slots_dict_with_range $start_slot $end_slot] + + set slot_stats [R 0 CLUSTER SLOT-STATS SLOTSRANGE $start_slot $end_slot] + assert_slot_visibility $slot_stats $expected_slots + } + + test "CLUSTER SLOT-STATS SLOTSRANGE some slots missing" { + set start_slot 100 + set end_slot 102 + set expected_slots [initialize_expected_slots_dict_with_range $start_slot $end_slot] + + R 0 CLUSTER DELSLOTS $start_slot + dict unset expected_slots $start_slot + + set slot_stats [R 0 CLUSTER SLOT-STATS SLOTSRANGE $start_slot $end_slot] + assert_slot_visibility $slot_stats $expected_slots + } +} + +# ----------------------------------------------------------------------------- +# Test cases for CLUSTER SLOT-STATS ORDERBY sub-argument. +# ----------------------------------------------------------------------------- + +start_cluster 1 0 {tags {external:skip cluster}} { + + # SET keys for target hashslots, to encourage ordering. + set hash_tags [list 0 1 2 3 4] + set num_keys 1 + foreach hash_tag $hash_tags { + for {set i 0} {$i < $num_keys} {incr i 1} { + R 0 SET "$i{$hash_tag}" VALUE + } + incr num_keys 1 + } + + # SET keys for random hashslots, for random noise. + set num_keys 0 + while {$num_keys < 1000} { + set random_key [randomInt 16384] + R 0 SET $random_key VALUE + incr num_keys 1 + } + + test "CLUSTER SLOT-STATS ORDERBY DESC correct ordering" { + set orderby "key-count" + set slot_stats [R 0 CLUSTER SLOT-STATS ORDERBY $orderby DESC] + assert_slot_stats_monotonic_descent $slot_stats $orderby + } + + test "CLUSTER SLOT-STATS ORDERBY ASC correct ordering" { + set orderby "key-count" + set slot_stats [R 0 CLUSTER SLOT-STATS ORDERBY $orderby ASC] + assert_slot_stats_monotonic_ascent $slot_stats $orderby + } + + test "CLUSTER SLOT-STATS ORDERBY LIMIT correct response pagination, where limit is less than number of assigned slots" { + R 0 FLUSHALL SYNC + + 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] + 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] + assert_slot_visibility $slot_stats_desc $expected_slots + assert_slot_visibility $slot_stats_asc $expected_slots + } + + test "CLUSTER SLOT-STATS ORDERBY LIMIT correct response pagination, where limit is greater than number of assigned slots" { + R 0 CONFIG SET cluster-require-full-coverage no + R 0 FLUSHALL SYNC + R 0 CLUSTER FLUSHSLOTS + R 0 CLUSTER ADDSLOTS 100 101 + + set num_assigned_slots 2 + 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 expected_response_length [expr min($num_assigned_slots, $limit)] + assert {$expected_response_length == $slot_stats_desc_length && $expected_response_length == $slot_stats_asc_length} + + set expected_slots [dict create 100 0 101 0] + assert_slot_visibility $slot_stats_desc $expected_slots + assert_slot_visibility $slot_stats_asc $expected_slots + } +} + +# ----------------------------------------------------------------------------- +# Test cases for CLUSTER SLOT-STATS replication. +# ----------------------------------------------------------------------------- + +start_cluster 1 1 {tags {external:skip cluster}} { + + # Define shared variables. + set key "FOO" + set key_slot [R 0 CLUSTER KEYSLOT $key] + + # Setup replication. + assert {[s -1 role] eq {slave}} + wait_for_condition 1000 50 { + [s -1 master_link_status] eq {up} + } else { + fail "Instance #1 master link status is not up" + } + R 1 readonly + + test "CLUSTER SLOT-STATS key-count replication for new keys" { + R 0 SET $key VALUE + set slot_stats_master [R 0 CLUSTER SLOT-STATS] + + set expected_slots_key_count [dict create $key_slot 1] + assert_slot_stats_key_count $slot_stats_master $expected_slots_key_count + wait_for_replica_key_exists $key 1 + + set slot_stats_replica [R 1 CLUSTER SLOT-STATS] + assert {$slot_stats_master eq $slot_stats_replica} + } + + test "CLUSTER SLOT-STATS key-count replication for existing keys" { + R 0 SET $key VALUE_UPDATED + set slot_stats_master [R 0 CLUSTER SLOT-STATS] + + set expected_slots_key_count [dict create $key_slot 1] + assert_slot_stats_key_count $slot_stats_master $expected_slots_key_count + wait_for_replica_key_exists $key 1 + + set slot_stats_replica [R 1 CLUSTER SLOT-STATS] + assert {$slot_stats_master eq $slot_stats_replica} + } + + test "CLUSTER SLOT-STATS key-count replication for deleting keys" { + R 0 DEL $key + set slot_stats_master [R 0 CLUSTER SLOT-STATS] + + set expected_slots_key_count [dict create $key_slot 0] + assert_slot_stats_key_count $slot_stats_master $expected_slots_key_count + wait_for_replica_key_exists $key 0 + + set slot_stats_replica [R 1 CLUSTER SLOT-STATS] + assert {$slot_stats_master eq $slot_stats_replica} + } +} \ No newline at end of file From 293fe674f45212619ddd0a6af473659ae943b3ef Mon Sep 17 00:00:00 2001 From: Kyle Kim Date: Thu, 13 Jun 2024 17:15:19 +0000 Subject: [PATCH 02/10] Minor revision. - Renamed cluster_slots.c to cluster_slot_stats.c - Renamed function signature and variables. - Removed 0 argument support. Signed-off-by: Kyle Kim --- src/Makefile | 2 +- src/cluster_slot_stats.c | 173 +++++++++++++++++++++ src/cluster_slots.c | 217 --------------------------- src/commands.def | 10 +- src/commands/cluster-slot-stats.json | 7 +- tests/unit/cluster/slot-stats.tcl | 24 +-- 6 files changed, 193 insertions(+), 240 deletions(-) create mode 100644 src/cluster_slot_stats.c delete mode 100644 src/cluster_slots.c diff --git a/src/Makefile b/src/Makefile index b55be582e2..5fc4ea31dd 100644 --- a/src/Makefile +++ b/src/Makefile @@ -383,7 +383,7 @@ endif ENGINE_NAME=valkey SERVER_NAME=$(ENGINE_NAME)-server$(PROG_SUFFIX) ENGINE_SENTINEL_NAME=$(ENGINE_NAME)-sentinel$(PROG_SUFFIX) -ENGINE_SERVER_OBJ=threads_mngr.o adlist.o quicklist.o ae.o anet.o dict.o kvstore.o server.o sds.o zmalloc.o lzf_c.o lzf_d.o pqsort.o zipmap.o sha1.o ziplist.o release.o networking.o util.o object.o db.o replication.o rdb.o t_string.o t_list.o t_set.o t_zset.o t_hash.o config.o aof.o pubsub.o multi.o debug.o sort.o intset.o syncio.o cluster.o cluster_legacy.o cluster_slots.o crc16.o endianconv.o slowlog.o eval.o bio.o rio.o rand.o memtest.o syscheck.o crcspeed.o crc64.o bitops.o sentinel.o notify.o setproctitle.o blocked.o hyperloglog.o latency.o sparkline.o valkey-check-rdb.o valkey-check-aof.o geo.o lazyfree.o module.o evict.o expire.o geohash.o geohash_helper.o childinfo.o defrag.o siphash.o rax.o t_stream.o listpack.o localtime.o lolwut.o lolwut5.o lolwut6.o acl.o tracking.o socket.o tls.o sha256.o timeout.o setcpuaffinity.o monotonic.o mt19937-64.o resp_parser.o call_reply.o script_lua.o script.o functions.o function_lua.o commands.o strl.o connection.o unix.o logreqres.o +ENGINE_SERVER_OBJ=threads_mngr.o adlist.o quicklist.o ae.o anet.o dict.o kvstore.o server.o sds.o zmalloc.o lzf_c.o lzf_d.o pqsort.o zipmap.o sha1.o ziplist.o release.o networking.o util.o object.o db.o replication.o rdb.o t_string.o t_list.o t_set.o t_zset.o t_hash.o config.o aof.o pubsub.o multi.o debug.o sort.o intset.o syncio.o cluster.o cluster_legacy.o cluster_slot_stats.o crc16.o endianconv.o slowlog.o eval.o bio.o rio.o rand.o memtest.o syscheck.o crcspeed.o crc64.o bitops.o sentinel.o notify.o setproctitle.o blocked.o hyperloglog.o latency.o sparkline.o valkey-check-rdb.o valkey-check-aof.o geo.o lazyfree.o module.o evict.o expire.o geohash.o geohash_helper.o childinfo.o defrag.o siphash.o rax.o t_stream.o listpack.o localtime.o lolwut.o lolwut5.o lolwut6.o acl.o tracking.o socket.o tls.o sha256.o timeout.o setcpuaffinity.o monotonic.o mt19937-64.o resp_parser.o call_reply.o script_lua.o script.o functions.o function_lua.o commands.o strl.o connection.o unix.o logreqres.o ENGINE_CLI_NAME=$(ENGINE_NAME)-cli$(PROG_SUFFIX) ENGINE_CLI_OBJ=anet.o adlist.o dict.o valkey-cli.o zmalloc.o release.o ae.o serverassert.o crcspeed.o crc64.o siphash.o crc16.o monotonic.o cli_common.o mt19937-64.o strl.o cli_commands.o ENGINE_BENCHMARK_NAME=$(ENGINE_NAME)-benchmark$(PROG_SUFFIX) diff --git a/src/cluster_slot_stats.c b/src/cluster_slot_stats.c new file mode 100644 index 0000000000..a66a7148ba --- /dev/null +++ b/src/cluster_slot_stats.c @@ -0,0 +1,173 @@ +/* + * Copyright Valkey Contributors. + * All rights reserved. + * SPDX-License-Identifier: BSD 3-Clause + */ + +#include "server.h" +#include "cluster.h" + +#define UNASSIGNED_SLOT 0 + +typedef enum { + KEY_COUNT, + INVALID, +} slotStatTypes; + +/* ----------------------------------------------------------------------------- + * CLUSTER SLOT-STATS command + * -------------------------------------------------------------------------- */ + +typedef struct { + int slot; + uint64_t stat; +} slotStatEntry; + +static int doesSlotBelongToMyShard(int slot) { + clusterNode *myself = getMyClusterNode(); + clusterNode *master = clusterNodeGetMaster(myself); + + return clusterNodeCoversSlot(master, slot); +} + +static void markSlotsAssignedToMyShard(unsigned char *assigned_slots, int start_slot, int end_slot, int *len) { + for (int slot = start_slot; slot <= end_slot; slot++) { + if (doesSlotBelongToMyShard(slot)) { + assigned_slots[slot]++; + (*len)++; + } + } +} + +static uint64_t getSlotStat(int slot, int stat_type) { + serverAssert(stat_type != INVALID); + uint64_t slot_stat = 0; + if (stat_type == KEY_COUNT) { + slot_stat = countKeysInSlot(slot); + } + return slot_stat; +} + +static int slotStatEntryAscCmp(const void *a, const void *b) { + slotStatEntry entry_a = *((slotStatEntry *) a); + slotStatEntry entry_b = *((slotStatEntry *) 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); + return entry_b.stat - entry_a.stat; +} + +static void collectAndSortSlotStats(slotStatEntry slot_stats[], int order_by, int desc) { + int i = 0; + + for (int slot = 0; slot < CLUSTER_SLOTS; slot++) { + if (doesSlotBelongToMyShard(slot)) { + slot_stats[i].slot = slot; + slot_stats[i].stat = getSlotStat(slot, order_by); + i++; + } + } + qsort(slot_stats, i, sizeof(slotStatEntry), (desc) ? slotStatEntryDescCmp : slotStatEntryAscCmp); +} + +static void addReplySlotStat(client *c, int slot) { + addReplyLongLong(c, slot); + addReplyMapLen(c, 1); + 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); + + for (int slot = startslot; slot <= endslot; slot++) { + if (assigned_slots[slot]) addReplySlotStat(c, slot); + } +} + +static void addReplySortedSlotStats(client *c, slotStatEntry slot_stats[], long limit) { + int num_slots_assigned = getMyShardSlotCount(); + int len = min(limit, num_slots_assigned); + addReplyMapLen(c, len); + + 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]; + collectAndSortSlotStats(slot_stats, order_by, desc); + addReplySortedSlotStats(c, slot_stats, limit); +} + +void clusterSlotStatsCommand(client *c) { + if (server.cluster_enabled == 0) { + addReplyError(c,"This instance has cluster support disabled"); + return; + } + + /* Parse additional arguments. */ + if (c->argc == 5 && !strcasecmp(c->argv[2]->ptr,"slotsrange")) { + /* CLUSTER SLOT-STATS SLOTSRANGE start-slot end-slot */ + int startslot, endslot; + if ((startslot = getSlotOrReply(c,c->argv[3])) == C_ERR || + (endslot = getSlotOrReply(c,c->argv[4])) == C_ERR) { + return; + } + if (startslot > endslot) { + addReplyErrorFormat(c,"Start slot number %d is greater than end slot number %d", startslot, endslot); + return; + } + /* Initialize slot assignment array. */ + unsigned char assigned_slots[CLUSTER_SLOTS]= {UNASSIGNED_SLOT}; + int len = 0; + markSlotsAssignedToMyShard(assigned_slots, startslot, endslot, &len); + addReplySlotStats(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] */ + int desc = 1, order_by = INVALID; + if (!strcasecmp(c->argv[3]->ptr, "key-count")) { + order_by = KEY_COUNT; + } else { + addReplyError(c, "Unrecognized sort metric for ORDER BY. The supported metrics are: key-count."); + return; + } + int i = 4; /* Next argument index, following ORDERBY */ + int limit_counter = 0, asc_desc_counter = 0; + long limit; + while(i < c->argc) { + int moreargs = c->argc > i+1; + if (!strcasecmp(c->argv[i]->ptr,"limit") && moreargs) { + if (getRangeLongFromObjectOrReply( + c, c->argv[i+1], 1, CLUSTER_SLOTS, &limit, + "Limit has to lie in between 1 and 16384 (maximum number of slots).") != C_OK) + return; + i++; + limit_counter++; + } else if (!strcasecmp(c->argv[i]->ptr,"asc")) { + desc = 0; + asc_desc_counter++; + } else if (!strcasecmp(c->argv[i]->ptr,"desc")) { + desc = 1; + asc_desc_counter++; + } else { + addReplyErrorObject(c,shared.syntaxerr); + return; + } + if (limit_counter > 1 || asc_desc_counter > 1) { + addReplyError(c, "Multiple filters of the same type are disallowed."); + return; + } + i++; + } + sortAndAddReplySlotStats(c, order_by, limit, desc); + + } else { + addReplySubcommandSyntaxError(c); + } +} diff --git a/src/cluster_slots.c b/src/cluster_slots.c deleted file mode 100644 index 7330c008d8..0000000000 --- a/src/cluster_slots.c +++ /dev/null @@ -1,217 +0,0 @@ -/* Cluster slots APIs and commands - to retrieve, update and process slot level data - * in association with Valkey cluster. - * - * Copyright (c) 2024, Kyle Kim , 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. - */ - -#include "server.h" -#include "cluster.h" -#include "cluster_legacy.h" - -#define DEFAULT_SLOT -1 -#define DEFAULT_STAT 0 -#define UNASSIGNED_SLOT 0 -#define ORDER_BY_KEY_COUNT 1 -#define ORDER_BY_INVALID -1 - -/* ----------------------------------------------------------------------------- - * CLUSTER SLOT-STATS command - * -------------------------------------------------------------------------- */ - -typedef struct sortedSlotStatEntry { - int slot; - uint64_t stat; -} sortedSlotStatEntry; - -static int doesSlotBelongToMyShard(int slot) { - clusterNode *n = clusterNodeGetMaster(server.cluster->myself); - return server.cluster->slots[slot] == n; -} - -static void markAssignedSlots(unsigned char *slots) { - for (int slot = 0; slot < CLUSTER_SLOTS; slot++) { - if (doesSlotBelongToMyShard(slot)) slots[slot]++; - } -} - -static int countAssignedSlotsFromSlotsArray(unsigned char *slots) { - int count = 0; - for (int slot = 0; slot < CLUSTER_SLOTS; slot++) { - if (slots[slot]) count++; - } - return count; -} - -static void checkSlotAssignment(unsigned char *slots, int start_slot, int end_slot) { - for (int slot = start_slot; slot <= end_slot; slot++) { - if (doesSlotBelongToMyShard(slot)) { - slots[slot]++; - } - } -} - -static uint64_t getSingleSlotStat(int slot, int order_by) { - serverAssert(order_by != ORDER_BY_INVALID); - uint64_t singleSlotStat = 0; - if (order_by == ORDER_BY_KEY_COUNT) { - singleSlotStat = countKeysInSlot(slot); - } - return singleSlotStat; -} - -static int slotStatEntryAscCmp(const void *a, const void *b) { - sortedSlotStatEntry entry_a = *((sortedSlotStatEntry *) a); - sortedSlotStatEntry entry_b = *((sortedSlotStatEntry *) b); - return entry_a.stat - entry_b.stat; -} - -static int slotStatEntryDescCmp(const void *a, const void *b) { - sortedSlotStatEntry entry_a = *((sortedSlotStatEntry *) a); - sortedSlotStatEntry entry_b = *((sortedSlotStatEntry *) b); - return entry_b.stat - entry_a.stat; -} - -static void sortSlotStats(sortedSlotStatEntry sorted[], int order_by, int desc) { - int i = 0; - - for (int slot = 0; slot < CLUSTER_SLOTS; slot++) { - if (doesSlotBelongToMyShard(slot)) { - sorted[i].slot = slot; - sorted[i].stat = getSingleSlotStat(slot, order_by); - i++; - } - } - qsort(sorted, i, sizeof(sortedSlotStatEntry), (desc) ? slotStatEntryDescCmp : slotStatEntryAscCmp); -} - -static void addReplySingleSlotStat(client *c, int slot) { - addReplyLongLong(c, slot); - addReplyMapLen(c, 1); - addReplyBulkCString(c, "key-count"); - addReplyLongLong(c, countKeysInSlot(slot)); -} - -static void addReplySlotStats(client *c, unsigned char *slots) { - int num_slots_assigned = countAssignedSlotsFromSlotsArray(slots); - addReplyMapLen(c, num_slots_assigned); - - for (int slot = 0; slot < CLUSTER_SLOTS; slot++) { - if (slots[slot]) addReplySingleSlotStat(c, slot); - } -} - -static void addReplySortedSlotStats(client *c, sortedSlotStatEntry sorted[], long limit) { - int num_slots_assigned = getMyShardSlotCount(); - int len = min(limit, num_slots_assigned); - addReplyMapLen(c, len); - - for (int i = 0; i < len; i++) { - addReplySingleSlotStat(c, sorted[i].slot); - } -} - -static void sortAndAddReplySlotStats(client *c, int order_by, long limit, int desc) { - sortedSlotStatEntry sorted[CLUSTER_SLOTS]; - sortSlotStats(sorted, order_by, desc); - addReplySortedSlotStats(c, sorted, limit); -} - -void clusterSlotStatsCommand(client *c) { - if (server.cluster_enabled == 0) { - addReplyError(c,"This instance has cluster support disabled"); - return; - } - - /* Initialize slot assignment array. */ - unsigned char slots[CLUSTER_SLOTS]= {UNASSIGNED_SLOT}; - - /* No further arguments. */ - if (c->argc == 2) { - /* CLUSTER SLOT-STATS */ - markAssignedSlots(slots); - addReplySlotStats(c, slots); - return; - } - - /* Parse additional arguments. */ - if (!strcasecmp(c->argv[2]->ptr,"slotsrange") && c->argc == 5) { - /* CLUSTER SLOT-STATS SLOTSRANGE start-slot end-slot */ - int startslot, endslot; - if ((startslot = getSlotOrReply(c,c->argv[3])) == C_ERR || - (endslot = getSlotOrReply(c,c->argv[4])) == C_ERR) { - return; - } - if (startslot > endslot) { - addReplyErrorFormat(c,"start slot number %d is greater than end slot number %d", startslot, endslot); - return; - } - 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] */ - int desc = 1, order_by = ORDER_BY_INVALID; - if (!strcasecmp(c->argv[3]->ptr, "key-count")) { - order_by = ORDER_BY_KEY_COUNT; - } else { - addReplyError(c, "unrecognized sort column for ORDER BY. The supported columns are: key-count."); - return; - } - int i = 4; /* Next argument index, following ORDERBY */ - int limit_counter = 0, asc_desc_counter = 0; - long limit; - while(i < c->argc) { - int moreargs = c->argc > i+1; - if (!strcasecmp(c->argv[i]->ptr,"limit") && moreargs) { - if (getRangeLongFromObjectOrReply( - c, c->argv[i+1], 1, CLUSTER_SLOTS, &limit, - "limit has to lie in between 1 and 16384 (maximum number of slots)") != C_OK) - return; - i++; - limit_counter++; - } else if (!strcasecmp(c->argv[i]->ptr,"asc")) { - desc = 0; - asc_desc_counter++; - } else if (!strcasecmp(c->argv[i]->ptr,"desc")) { - desc = 1; - asc_desc_counter++; - } else { - addReplyErrorObject(c,shared.syntaxerr); - return; - } - - if (limit_counter > 1 || asc_desc_counter > 1) { - addReplyError(c, "you cannot provide multiple filters of the same type."); - return; - } - i++; - } - sortAndAddReplySlotStats(c, order_by, limit, desc); - } else { - addReplySubcommandSyntaxError(c); - } -} diff --git a/src/commands.def b/src/commands.def index 9208f26553..1e64abd550 100644 --- a/src/commands.def +++ b/src/commands.def @@ -955,20 +955,20 @@ struct COMMAND_ARG CLUSTER_SLOT_STATS_filter_orderby_order_Subargs[] = { /* CLUSTER SLOT_STATS filter orderby argument table */ struct COMMAND_ARG CLUSTER_SLOT_STATS_filter_orderby_Subargs[] = { -{MAKE_ARG("column",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("metric",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, {MAKE_ARG("limit",ARG_TYPE_INTEGER,-1,"LIMIT",NULL,NULL,CMD_ARG_OPTIONAL,0,NULL)}, {MAKE_ARG("order",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,2,NULL),.subargs=CLUSTER_SLOT_STATS_filter_orderby_order_Subargs}, }; /* CLUSTER SLOT_STATS filter argument table */ struct COMMAND_ARG CLUSTER_SLOT_STATS_filter_Subargs[] = { -{MAKE_ARG("slotsrange",ARG_TYPE_BLOCK,-1,"SLOTSRANGE",NULL,NULL,CMD_ARG_OPTIONAL,2,NULL),.subargs=CLUSTER_SLOT_STATS_filter_slotsrange_Subargs}, -{MAKE_ARG("orderby",ARG_TYPE_BLOCK,-1,"ORDERBY",NULL,NULL,CMD_ARG_OPTIONAL,3,NULL),.subargs=CLUSTER_SLOT_STATS_filter_orderby_Subargs}, +{MAKE_ARG("slotsrange",ARG_TYPE_BLOCK,-1,"SLOTSRANGE",NULL,NULL,CMD_ARG_NONE,2,NULL),.subargs=CLUSTER_SLOT_STATS_filter_slotsrange_Subargs}, +{MAKE_ARG("orderby",ARG_TYPE_BLOCK,-1,"ORDERBY",NULL,NULL,CMD_ARG_NONE,3,NULL),.subargs=CLUSTER_SLOT_STATS_filter_orderby_Subargs}, }; /* CLUSTER SLOT_STATS argument table */ struct COMMAND_ARG CLUSTER_SLOT_STATS_Args[] = { -{MAKE_ARG("filter",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,2,NULL),.subargs=CLUSTER_SLOT_STATS_filter_Subargs}, +{MAKE_ARG("filter",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_NONE,2,NULL),.subargs=CLUSTER_SLOT_STATS_filter_Subargs}, }; /********** CLUSTER SLOTS ********************/ @@ -1022,7 +1022,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,0,CLUSTER_SETSLOT_Tips,0,clusterCommand,-4,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_STALE,0,CLUSTER_SETSLOT_Keyspecs,0,NULL,2),.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 master 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.","7.2.6",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_SLOT_STATS_History,0,CLUSTER_SLOT_STATS_Tips,2,clusterSlotStatsCommand,-2,CMD_STALE|CMD_LOADING,0,CLUSTER_SLOT_STATS_Keyspecs,0,NULL,1),.args=CLUSTER_SLOT_STATS_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,-2,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_DEPRECATED,"`CLUSTER SHARDS`","7.0.0","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 index 2f1aebb67e..331b056022 100644 --- a/src/commands/cluster-slot-stats.json +++ b/src/commands/cluster-slot-stats.json @@ -3,7 +3,7 @@ "summary": "Return 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": "7.2.6", + "since": "8.0.0", "arity": -2, "container": "CLUSTER", "function": "clusterSlotStatsCommand", @@ -19,13 +19,11 @@ { "name": "filter", "type": "oneof", - "optional": true, "arguments": [ { "token": "SLOTSRANGE", "name": "slotsrange", "type": "block", - "optional": true, "arguments": [ { "name": "start-slot", @@ -41,10 +39,9 @@ "token": "ORDERBY", "name": "orderby", "type": "block", - "optional": true, "arguments": [ { - "name": "column", + "name": "metric", "type": "string" }, { diff --git a/tests/unit/cluster/slot-stats.tcl b/tests/unit/cluster/slot-stats.tcl index 524efed885..181d706f60 100644 --- a/tests/unit/cluster/slot-stats.tcl +++ b/tests/unit/cluster/slot-stats.tcl @@ -107,25 +107,25 @@ start_cluster 1 0 {tags {external:skip cluster}} { set expected_slots_to_key_count [dict create $key_slot 1] test "CLUSTER SLOT-STATS contains default value upon valkey-server startup" { - set slot_stats [R 0 CLUSTER SLOT-STATS] + set slot_stats [R 0 CLUSTER SLOT-STATS SLOTSRANGE 0 16383] assert_empty_slot_stats $slot_stats } test "CLUSTER SLOT-STATS contains correct metrics upon key introduction" { R 0 SET $key TEST - set slot_stats [R 0 CLUSTER SLOT-STATS] + set slot_stats [R 0 CLUSTER SLOT-STATS SLOTSRANGE 0 16383] assert_empty_slot_stats_with_exception $slot_stats $expected_slots_to_key_count } test "CLUSTER SLOT-STATS contains correct metrics upon key mutation" { R 0 SET $key NEW_VALUE - set slot_stats [R 0 CLUSTER SLOT-STATS] + set slot_stats [R 0 CLUSTER SLOT-STATS SLOTSRANGE 0 16383] assert_empty_slot_stats_with_exception $slot_stats $expected_slots_to_key_count } test "CLUSTER SLOT-STATS contains correct metrics upon key deletion" { R 0 DEL $key - set slot_stats [R 0 CLUSTER SLOT-STATS] + set slot_stats [R 0 CLUSTER SLOT-STATS SLOTSRANGE 0 16383] assert_empty_slot_stats $slot_stats } @@ -135,13 +135,13 @@ start_cluster 1 0 {tags {external:skip cluster}} { R 0 CLUSTER DELSLOTS $key_slot set expected_slots [initialize_expected_slots_dict] dict unset expected_slots $key_slot - set slot_stats [R 0 CLUSTER SLOT-STATS] + set slot_stats [R 0 CLUSTER SLOT-STATS SLOTSRANGE 0 16383] assert {[dict size $expected_slots] == 16383} assert_slot_visibility $slot_stats $expected_slots R 0 CLUSTER ADDSLOTS $key_slot set expected_slots [initialize_expected_slots_dict] - set slot_stats [R 0 CLUSTER SLOT-STATS] + set slot_stats [R 0 CLUSTER SLOT-STATS SLOTSRANGE 0 16383] assert {[dict size $expected_slots] == 16384} assert_slot_visibility $slot_stats $expected_slots } @@ -268,37 +268,37 @@ start_cluster 1 1 {tags {external:skip cluster}} { test "CLUSTER SLOT-STATS key-count replication for new keys" { R 0 SET $key VALUE - set slot_stats_master [R 0 CLUSTER SLOT-STATS] + set slot_stats_master [R 0 CLUSTER SLOT-STATS SLOTSRANGE 0 16383] set expected_slots_key_count [dict create $key_slot 1] assert_slot_stats_key_count $slot_stats_master $expected_slots_key_count wait_for_replica_key_exists $key 1 - set slot_stats_replica [R 1 CLUSTER SLOT-STATS] + set slot_stats_replica [R 1 CLUSTER SLOT-STATS SLOTSRANGE 0 16383] assert {$slot_stats_master eq $slot_stats_replica} } test "CLUSTER SLOT-STATS key-count replication for existing keys" { R 0 SET $key VALUE_UPDATED - set slot_stats_master [R 0 CLUSTER SLOT-STATS] + set slot_stats_master [R 0 CLUSTER SLOT-STATS SLOTSRANGE 0 16383] set expected_slots_key_count [dict create $key_slot 1] assert_slot_stats_key_count $slot_stats_master $expected_slots_key_count wait_for_replica_key_exists $key 1 - set slot_stats_replica [R 1 CLUSTER SLOT-STATS] + set slot_stats_replica [R 1 CLUSTER SLOT-STATS SLOTSRANGE 0 16383] assert {$slot_stats_master eq $slot_stats_replica} } test "CLUSTER SLOT-STATS key-count replication for deleting keys" { R 0 DEL $key - set slot_stats_master [R 0 CLUSTER SLOT-STATS] + set slot_stats_master [R 0 CLUSTER SLOT-STATS SLOTSRANGE 0 16383] set expected_slots_key_count [dict create $key_slot 0] assert_slot_stats_key_count $slot_stats_master $expected_slots_key_count wait_for_replica_key_exists $key 0 - set slot_stats_replica [R 1 CLUSTER SLOT-STATS] + set slot_stats_replica [R 1 CLUSTER SLOT-STATS SLOTSRANGE 0 16383] assert {$slot_stats_master eq $slot_stats_replica} } } \ No newline at end of file From 494af2c6d0eed4e4d150594b20ce51514e71b54f Mon Sep 17 00:00:00 2001 From: Kyle Kim Date: Mon, 24 Jun 2024 15:29:08 +0000 Subject: [PATCH 03/10] Minor revision. - Update reply_schema. Signed-off-by: Kyle Kim --- src/commands/cluster-slot-stats.json | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/commands/cluster-slot-stats.json b/src/commands/cluster-slot-stats.json index 331b056022..5e22146d5e 100644 --- a/src/commands/cluster-slot-stats.json +++ b/src/commands/cluster-slot-stats.json @@ -15,6 +15,13 @@ "NONDETERMINISTIC_OUTPUT", "ALL_SHARDS" ], + "reply_schema": { + "type": "object", + "description": "Map of slots and their respective usage statistics.", + "additionalProperties": { + "type": "string" + } + }, "arguments": [ { "name": "filter", From 5f34353d470565903313fbfc40af5e79b2161d85 Mon Sep 17 00:00:00 2001 From: Kyle Kim Date: Mon, 24 Jun 2024 18:58:20 +0000 Subject: [PATCH 04/10] Minor revision. - Fixed origin/unstable merge conflicts. - Updated formatting. - Updated cluster-slot-stats.json. Signed-off-by: Kyle Kim --- src/cluster_slot_stats.c | 14 +++++++------- src/commands.def | 4 ++-- src/commands/cluster-slot-stats.json | 6 +++--- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/cluster_slot_stats.c b/src/cluster_slot_stats.c index d03bd811a5..7558032812 100644 --- a/src/cluster_slot_stats.c +++ b/src/cluster_slot_stats.c @@ -106,12 +106,12 @@ static void sortAndAddReplySlotStats(client *c, int order_by, long limit, int de void clusterSlotStatsCommand(client *c) { if (server.cluster_enabled == 0) { - addReplyError(c,"This instance has cluster support disabled"); + addReplyError(c, "This instance has cluster support disabled"); return; } /* Parse additional arguments. */ - if (c->argc == 5 && !strcasecmp(c->argv[2]->ptr,"slotsrange")) { + if (c->argc == 5 && !strcasecmp(c->argv[2]->ptr, "slotsrange")) { /* CLUSTER SLOT-STATS SLOTSRANGE start-slot end-slot */ int startslot, endslot; if ((startslot = getSlotOrReply(c,c->argv[3])) == C_ERR || @@ -119,7 +119,7 @@ void clusterSlotStatsCommand(client *c) { return; } if (startslot > endslot) { - addReplyErrorFormat(c,"Start slot number %d is greater than end slot number %d", startslot, endslot); + addReplyErrorFormat(c, "Start slot number %d is greater than end slot number %d", startslot, endslot); return; } /* Initialize slot assignment array. */ @@ -128,7 +128,7 @@ void clusterSlotStatsCommand(client *c) { markSlotsAssignedToMyShard(assigned_slots, startslot, endslot, &len); addReplySlotStats(c, assigned_slots, startslot, endslot, len); - } else if (c->argc >= 4 && !strcasecmp(c->argv[2]->ptr,"orderby")) { + } else if (c->argc >= 4 && !strcasecmp(c->argv[2]->ptr, "orderby")) { /* CLUSTER SLOT-STATS ORDERBY metric [LIMIT limit] [ASC | DESC] */ int desc = 1, order_by = INVALID; if (!strcasecmp(c->argv[3]->ptr, "key-count")) { @@ -142,17 +142,17 @@ void clusterSlotStatsCommand(client *c) { long limit; while(i < c->argc) { int moreargs = c->argc > i+1; - if (!strcasecmp(c->argv[i]->ptr,"limit") && moreargs) { + if (!strcasecmp(c->argv[i]->ptr, "limit") && moreargs) { if (getRangeLongFromObjectOrReply( c, c->argv[i+1], 1, CLUSTER_SLOTS, &limit, "Limit has to lie in between 1 and 16384 (maximum number of slots).") != C_OK) return; i++; limit_counter++; - } else if (!strcasecmp(c->argv[i]->ptr,"asc")) { + } else if (!strcasecmp(c->argv[i]->ptr, "asc")) { desc = 0; asc_desc_counter++; - } else if (!strcasecmp(c->argv[i]->ptr,"desc")) { + } else if (!strcasecmp(c->argv[i]->ptr, "desc")) { desc = 1; asc_desc_counter++; } else { diff --git a/src/commands.def b/src/commands.def index 39fad65bb7..f8ef1c20fe 100644 --- a/src/commands.def +++ b/src/commands.def @@ -941,7 +941,7 @@ struct COMMAND_ARG CLUSTER_SLAVES_Args[] = { /* CLUSTER SLOT_STATS tips */ const char *CLUSTER_SLOT_STATS_Tips[] = { "nondeterministic_output", -"all_shards", +"request_policy:all_shards", }; #endif @@ -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,-2,CMD_STALE|CMD_LOADING,0,CLUSTER_SLOT_STATS_Keyspecs,0,NULL,1),.args=CLUSTER_SLOT_STATS_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("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 index 5e22146d5e..193cfe7bb8 100644 --- a/src/commands/cluster-slot-stats.json +++ b/src/commands/cluster-slot-stats.json @@ -1,10 +1,10 @@ { "SLOT-STATS": { "summary": "Return 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.", + "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", - "arity": -2, + "arity": -4, "container": "CLUSTER", "function": "clusterSlotStatsCommand", "command_flags": [ @@ -13,7 +13,7 @@ ], "command_tips": [ "NONDETERMINISTIC_OUTPUT", - "ALL_SHARDS" + "REQUEST_POLICY:ALL_SHARDS" ], "reply_schema": { "type": "object", From 492b758c80ce979f287d410b8ae470e7228a55ef Mon Sep 17 00:00:00 2001 From: Kyle Kim Date: Mon, 24 Jun 2024 19:21:15 +0000 Subject: [PATCH 05/10] Minor revision. - Fixed format based on clang-format. - Re-added crccombine.o into Makefile. Signed-off-by: Kyle Kim --- src/Makefile | 2 +- src/cluster_slot_stats.c | 26 +++++++++++++------------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/Makefile b/src/Makefile index 2ca0f69f8c..3f78657b2f 100644 --- a/src/Makefile +++ b/src/Makefile @@ -401,7 +401,7 @@ endif ENGINE_NAME=valkey SERVER_NAME=$(ENGINE_NAME)-server$(PROG_SUFFIX) ENGINE_SENTINEL_NAME=$(ENGINE_NAME)-sentinel$(PROG_SUFFIX) -ENGINE_SERVER_OBJ=threads_mngr.o adlist.o quicklist.o ae.o anet.o dict.o kvstore.o server.o sds.o zmalloc.o lzf_c.o lzf_d.o pqsort.o zipmap.o sha1.o ziplist.o release.o networking.o util.o object.o db.o replication.o rdb.o t_string.o t_list.o t_set.o t_zset.o t_hash.o config.o aof.o pubsub.o multi.o debug.o sort.o intset.o syncio.o cluster.o cluster_legacy.o cluster_slot_stats.o crc16.o endianconv.o slowlog.o eval.o bio.o rio.o rand.o memtest.o syscheck.o crcspeed.o crc64.o bitops.o sentinel.o notify.o setproctitle.o blocked.o hyperloglog.o latency.o sparkline.o valkey-check-rdb.o valkey-check-aof.o geo.o lazyfree.o module.o evict.o expire.o geohash.o geohash_helper.o childinfo.o defrag.o siphash.o rax.o t_stream.o listpack.o localtime.o lolwut.o lolwut5.o lolwut6.o acl.o tracking.o socket.o tls.o sha256.o timeout.o setcpuaffinity.o monotonic.o mt19937-64.o resp_parser.o call_reply.o script_lua.o script.o functions.o function_lua.o commands.o strl.o connection.o unix.o logreqres.o +ENGINE_SERVER_OBJ=threads_mngr.o adlist.o quicklist.o ae.o anet.o dict.o kvstore.o server.o sds.o zmalloc.o lzf_c.o lzf_d.o pqsort.o zipmap.o sha1.o ziplist.o release.o networking.o util.o object.o db.o replication.o rdb.o t_string.o t_list.o t_set.o t_zset.o t_hash.o config.o aof.o pubsub.o multi.o debug.o sort.o intset.o syncio.o cluster.o cluster_legacy.o cluster_slot_stats.o crc16.o endianconv.o slowlog.o eval.o bio.o rio.o rand.o memtest.o syscheck.o crcspeed.o crccombine.o crc64.o bitops.o sentinel.o notify.o setproctitle.o blocked.o hyperloglog.o latency.o sparkline.o valkey-check-rdb.o valkey-check-aof.o geo.o lazyfree.o module.o evict.o expire.o geohash.o geohash_helper.o childinfo.o defrag.o siphash.o rax.o t_stream.o listpack.o localtime.o lolwut.o lolwut5.o lolwut6.o acl.o tracking.o socket.o tls.o sha256.o timeout.o setcpuaffinity.o monotonic.o mt19937-64.o resp_parser.o call_reply.o script_lua.o script.o functions.o function_lua.o commands.o strl.o connection.o unix.o logreqres.o ENGINE_CLI_NAME=$(ENGINE_NAME)-cli$(PROG_SUFFIX) ENGINE_CLI_OBJ=anet.o adlist.o dict.o valkey-cli.o zmalloc.o release.o ae.o serverassert.o crcspeed.o crccombine.o crc64.o siphash.o crc16.o monotonic.o cli_common.o mt19937-64.o strl.o cli_commands.o ENGINE_BENCHMARK_NAME=$(ENGINE_NAME)-benchmark$(PROG_SUFFIX) diff --git a/src/cluster_slot_stats.c b/src/cluster_slot_stats.c index 7558032812..74889e9117 100644 --- a/src/cluster_slot_stats.c +++ b/src/cluster_slot_stats.c @@ -49,20 +49,20 @@ static uint64_t getSlotStat(int slot, int stat_type) { } static int slotStatEntryAscCmp(const void *a, const void *b) { - slotStatEntry entry_a = *((slotStatEntry *) a); - slotStatEntry entry_b = *((slotStatEntry *) b); + slotStatEntry entry_a = *((slotStatEntry *)a); + slotStatEntry entry_b = *((slotStatEntry *)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); + slotStatEntry entry_a = *((slotStatEntry *)a); + slotStatEntry entry_b = *((slotStatEntry *)b); return entry_b.stat - entry_a.stat; } static void collectAndSortSlotStats(slotStatEntry slot_stats[], int order_by, int desc) { int i = 0; - + for (int slot = 0; slot < CLUSTER_SLOTS; slot++) { if (doesSlotBelongToMyShard(slot)) { slot_stats[i].slot = slot; @@ -114,8 +114,8 @@ void clusterSlotStatsCommand(client *c) { if (c->argc == 5 && !strcasecmp(c->argv[2]->ptr, "slotsrange")) { /* CLUSTER SLOT-STATS SLOTSRANGE start-slot end-slot */ int startslot, endslot; - if ((startslot = getSlotOrReply(c,c->argv[3])) == C_ERR || - (endslot = getSlotOrReply(c,c->argv[4])) == C_ERR) { + if ((startslot = getSlotOrReply(c, c->argv[3])) == C_ERR || + (endslot = getSlotOrReply(c, c->argv[4])) == C_ERR) { return; } if (startslot > endslot) { @@ -123,7 +123,7 @@ void clusterSlotStatsCommand(client *c) { return; } /* Initialize slot assignment array. */ - unsigned char assigned_slots[CLUSTER_SLOTS]= {UNASSIGNED_SLOT}; + unsigned char assigned_slots[CLUSTER_SLOTS] = {UNASSIGNED_SLOT}; int len = 0; markSlotsAssignedToMyShard(assigned_slots, startslot, endslot, &len); addReplySlotStats(c, assigned_slots, startslot, endslot, len); @@ -140,12 +140,12 @@ void clusterSlotStatsCommand(client *c) { int i = 4; /* Next argument index, following ORDERBY */ int limit_counter = 0, asc_desc_counter = 0; long limit; - while(i < c->argc) { - int moreargs = c->argc > i+1; + while (i < c->argc) { + int moreargs = c->argc > i + 1; if (!strcasecmp(c->argv[i]->ptr, "limit") && moreargs) { if (getRangeLongFromObjectOrReply( - c, c->argv[i+1], 1, CLUSTER_SLOTS, &limit, - "Limit has to lie in between 1 and 16384 (maximum number of slots).") != C_OK) + c, c->argv[i + 1], 1, CLUSTER_SLOTS, &limit, + "Limit has to lie in between 1 and 16384 (maximum number of slots).") != C_OK) return; i++; limit_counter++; @@ -156,7 +156,7 @@ void clusterSlotStatsCommand(client *c) { desc = 1; asc_desc_counter++; } else { - addReplyErrorObject(c,shared.syntaxerr); + addReplyErrorObject(c, shared.syntaxerr); return; } if (limit_counter > 1 || asc_desc_counter > 1) { From 12730f0c2e2086a49308c43e4859c2a491bd21f1 Mon Sep 17 00:00:00 2001 From: Kyle Kim Date: Wed, 26 Jun 2024 02:55:28 +0000 Subject: [PATCH 06/10] Minor revision. - Updated RESP reply from map to array. - Renamed slotStatEntry to slotStatForSort. Signed-off-by: Kyle Kim --- src/cluster_slot_stats.c | 42 ++++++++++++++++------------ src/commands.def | 2 +- src/commands/cluster-slot-stats.json | 14 ++++++---- tests/unit/cluster/slot-stats.tcl | 30 ++++++++++++++++---- 4 files changed, 59 insertions(+), 29 deletions(-) diff --git a/src/cluster_slot_stats.c b/src/cluster_slot_stats.c index 74889e9117..90748510a1 100644 --- 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 index f8ef1c20fe..4a72c0b4c1 100644 --- 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 index 193cfe7bb8..4fe6d70a76 100644 --- 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 index 181d706f60..97d8519cee 100644 --- 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} From 6921c026eee45693e1a6f7d7dbfe30fa3a837da3 Mon Sep 17 00:00:00 2001 From: Kyle Kim Date: Thu, 27 Jun 2024 20:46:35 +0000 Subject: [PATCH 07/10] Minor revision - Updated the RESP3 response from array of maps to array of arrays. Signed-off-by: Kyle Kim --- src/cluster_slot_stats.c | 3 ++- src/commands/cluster-slot-stats.json | 14 +++++++++----- tests/unit/cluster/slot-stats.tcl | 7 +++---- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/cluster_slot_stats.c b/src/cluster_slot_stats.c index 90748510a1..c53ab659b5 100644 --- a/src/cluster_slot_stats.c +++ b/src/cluster_slot_stats.c @@ -75,7 +75,8 @@ static void collectAndSortSlotStats(slotStatForSort slot_stats[], int order_by, } static void addReplySlotStat(client *c, int slot) { - addReplyMapLen(c, 1); /* Map for (int) slot to (map) usage statistics. */ + addReplyArrayLen(c, 2); /* Array of size 2, where 0th index represents (int) slot, + * and 1st index represents (map) usage statistics. */ addReplyLongLong(c, slot); addReplyMapLen(c, 1); /* Nested map representing slot usage statistics. */ addReplyBulkCString(c, "key-count"); diff --git a/src/commands/cluster-slot-stats.json b/src/commands/cluster-slot-stats.json index 4fe6d70a76..ebbd5d1982 100644 --- a/src/commands/cluster-slot-stats.json +++ b/src/commands/cluster-slot-stats.json @@ -17,12 +17,16 @@ ], "reply_schema": { "type": "array", - "description": "Array of nested maps, where each map represents a slot and its respective usage statistics.", + "description": "Array of nested arrays, where the inner array element represents a slot and its respective usage statistics.", "items": { - "type": "object", - "description": "Map of a slot and its respective usage statistics.", - "additionalProperties": { - "type": "string" + "type": "array", + "description": "Array of size 2, where 0th index represents (int) slot, and 1st index represents (map) usage statistics.", + "items": { + "type": "object", + "description": "Map of slot usage statistics.", + "additionalProperties": { + "type": "string" + } } } }, diff --git a/tests/unit/cluster/slot-stats.tcl b/tests/unit/cluster/slot-stats.tcl index 97d8519cee..c2923dc8bb 100644 --- a/tests/unit/cluster/slot-stats.tcl +++ b/tests/unit/cluster/slot-stats.tcl @@ -9,10 +9,9 @@ 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 - } + # slot_stat is an array of size 2, where 0th index represents (int) slot, + # and 1st index represents (map) usage statistics. + dict set res [lindex $slot_stat 0] [lindex $slot_stat 1] } return $res } From a9535f7c0387d59721cb373cd631610e05447d74 Mon Sep 17 00:00:00 2001 From: Madelyn Olson Date: Thu, 27 Jun 2024 16:16:45 -0700 Subject: [PATCH 08/10] Update src/commands/cluster-slot-stats.json Signed-off-by: Madelyn Olson --- src/commands/cluster-slot-stats.json | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/commands/cluster-slot-stats.json b/src/commands/cluster-slot-stats.json index ebbd5d1982..f82f256462 100644 --- a/src/commands/cluster-slot-stats.json +++ b/src/commands/cluster-slot-stats.json @@ -20,14 +20,22 @@ "description": "Array of nested arrays, where the inner array element represents a slot and its respective usage statistics.", "items": { "type": "array", - "description": "Array of size 2, where 0th index represents (int) slot, and 1st index represents (map) usage statistics.", - "items": { - "type": "object", - "description": "Map of slot usage statistics.", - "additionalProperties": { - "type": "string" + "description": "Array of size 2, where 0th index represents (int) slot and 1st index represents (map) usage statistics.", + "minItems": 2, + "maxItems": 2, + "items": [ + { + "description": "Slot Number.", + "type": "integer" + }, + { + "type": "object", + "description": "Map of slot usage statistics.", + "additionalProperties": { + "type": "string" + } } - } + ] } }, "arguments": [ From 8e1105f61978ca6e3118f75bf981b4f127e9d6df Mon Sep 17 00:00:00 2001 From: Madelyn Olson Date: Thu, 27 Jun 2024 16:26:12 -0700 Subject: [PATCH 09/10] Update src/commands/cluster-slot-stats.json Signed-off-by: Madelyn Olson --- src/commands/cluster-slot-stats.json | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/commands/cluster-slot-stats.json b/src/commands/cluster-slot-stats.json index f82f256462..7dfcd415ec 100644 --- a/src/commands/cluster-slot-stats.json +++ b/src/commands/cluster-slot-stats.json @@ -31,8 +31,11 @@ { "type": "object", "description": "Map of slot usage statistics.", - "additionalProperties": { - "type": "string" + "additionalProperties": false, + "properties": { + "key-count": { + "type": "integer" + } } } ] From 3cf86ab6ed9ee808d2d08727a25dc786befa45db Mon Sep 17 00:00:00 2001 From: Madelyn Olson Date: Thu, 27 Jun 2024 16:35:31 -0700 Subject: [PATCH 10/10] Apply suggestions from code review Signed-off-by: Madelyn Olson --- src/cluster_slot_stats.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/cluster_slot_stats.c b/src/cluster_slot_stats.c index c53ab659b5..515be588f7 100644 --- a/src/cluster_slot_stats.c +++ b/src/cluster_slot_stats.c @@ -26,18 +26,20 @@ typedef struct { static int doesSlotBelongToMyShard(int slot) { clusterNode *myself = getMyClusterNode(); - clusterNode *master = clusterNodeGetPrimary(myself); + clusterNode *primary = clusterNodeGetPrimary(myself); - return clusterNodeCoversSlot(master, slot); + return clusterNodeCoversSlot(primary, slot); } -static void markSlotsAssignedToMyShard(unsigned char *assigned_slots, int start_slot, int end_slot, int *len) { +static int markSlotsAssignedToMyShard(unsigned char *assigned_slots, int start_slot, int end_slot) { + int assigned_slots_count = 0; for (int slot = start_slot; slot <= end_slot; slot++) { if (doesSlotBelongToMyShard(slot)) { assigned_slots[slot]++; - (*len)++; + assigned_slots_count++; } } + return assigned_slots_count; } static uint64_t getSlotStat(int slot, int stat_type) { @@ -131,9 +133,8 @@ void clusterSlotStatsCommand(client *c) { } /* Initialize slot assignment array. */ unsigned char assigned_slots[CLUSTER_SLOTS] = {UNASSIGNED_SLOT}; - int len = 0; - markSlotsAssignedToMyShard(assigned_slots, startslot, endslot, &len); - addReplySlotsRange(c, assigned_slots, startslot, endslot, len); + int assigned_slots_count = markSlotsAssignedToMyShard(assigned_slots, startslot, endslot); + addReplySlotsRange(c, assigned_slots, startslot, endslot, assigned_slots_count); } else if (c->argc >= 4 && !strcasecmp(c->argv[2]->ptr, "orderby")) { /* CLUSTER SLOT-STATS ORDERBY metric [LIMIT limit] [ASC | DESC] */ @@ -152,8 +153,9 @@ void clusterSlotStatsCommand(client *c) { if (!strcasecmp(c->argv[i]->ptr, "limit") && moreargs) { if (getRangeLongFromObjectOrReply( c, c->argv[i + 1], 1, CLUSTER_SLOTS, &limit, - "Limit has to lie in between 1 and 16384 (maximum number of slots).") != C_OK) + "Limit has to lie in between 1 and 16384 (maximum number of slots).") != C_OK) { return; + } i++; limit_counter++; } else if (!strcasecmp(c->argv[i]->ptr, "asc")) {