-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce CLUSTER SLOT-STATS command (#20). #351
base: unstable
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -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 <kimkyle at amazon dot com>, Amazon Web Services. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* All rights reserved. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* Redistribution and use in source and binary forms, with or without | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* modification, are permitted provided that the following conditions are met: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* * Redistributions of source code must retain the above copyright notice, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* this list of conditions and the following disclaimer. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* * Redistributions in binary form must reproduce the above copyright | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* notice, this list of conditions and the following disclaimer in the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* documentation and/or other materials provided with the distribution. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* * Neither the name of Valkey nor the names of its contributors may be used | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* to endorse or promote products derived from this software without | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* specific prior written permission. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* POSSIBILITY OF SUCH DAMAGE. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+4
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We live in the future where you get to write less stuff. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#include "server.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#include "cluster.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#include "cluster_legacy.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+33
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you need from cluster_legacy.h? We have some vague notion that we want to better abstract away cluster_legacy unless it's dealing with the clusterbus and/or low-level topology. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Moving forward, we won't have direct access to |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#define DEFAULT_SLOT -1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this used for? Either remove or document it. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#define DEFAULT_STAT 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this used for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will be removed. These are residues of per-slot CPU metrics, which is for the upcoming PR. Repeats below. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#define UNASSIGNED_SLOT 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#define ORDER_BY_KEY_COUNT 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#define ORDER_BY_INVALID -1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+39
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think enum is better for these order options (and future extensions) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/* ----------------------------------------------------------------------------- | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* CLUSTER SLOT-STATS command | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* -------------------------------------------------------------------------- */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typedef struct sortedSlotStatEntry { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit. For non-recursive struct definitions, we can save some typing by omitting the struct identifier
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
int slot; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
uint64_t stat; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} sortedSlotStatEntry; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the struct itself has the sorted property.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
static int doesSlotBelongToMyShard(int slot) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
clusterNode *n = clusterNodeGetMaster(server.cluster->myself); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return server.cluster->slots[slot] == n; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
static void markAssignedSlots(unsigned char *slots) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use a more descriptive name?
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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++; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if a bitmap is used, there is a more efficient way to count number set bits in the bitmap using bit operations. https://graphics.stanford.edu/~seander/bithacks.html#CountBitsSetNaive |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return count; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
static void checkSlotAssignment(unsigned char *slots, int start_slot, int end_slot) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should expose also |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
for (int slot = start_slot; slot <= end_slot; slot++) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (doesSlotBelongToMyShard(slot)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
slots[slot]++; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
static uint64_t getSingleSlotStat(int slot, int order_by) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any significance of "single" here?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think "order_by" is relevant in this function. This implementation returns a specific stat based on the given type but there is no ordering being performed.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
serverAssert(order_by != ORDER_BY_INVALID); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
uint64_t singleSlotStat = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For variables, snake casing is more common. camel casing is for functions and structs in Valkey |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - "sorted" is the property of the function rather than that of the array.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
int i = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
for (int slot = 0; slot < CLUSTER_SLOTS; slot++) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (doesSlotBelongToMyShard(slot)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this can be tested more efficiently via bitmapTestBit - need to first abstract out a proper interface in
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - "sorted" is not relevant in the context of this function
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nt - this function does more than sorting.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
addReplySortedSlotStats(c, sorted, limit); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
void clusterSlotStatsCommand(client *c) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the convention is
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (server.cluster_enabled == 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
addReplyError(c,"This instance has cluster support disabled"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/* Initialize slot assignment array. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
unsigned char slots[CLUSTER_SLOTS]= {UNASSIGNED_SLOT}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can save space by using a bitmap instead and in fact this bitmap is already available in |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/* No further arguments. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (c->argc == 2) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/* CLUSTER SLOT-STATS */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
markAssignedSlots(slots); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
addReplySlotStats(c, slots); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+153
to
+159
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have we ever called this operationally? I feel like we always call it filtered. Maybe we should move away from a foot gun and encourage people to limit the amount of data they pull by deleting the 2 arg form? Not a strong opinion, just something I thought about since I'm taking a fresh look. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I like the idea of explicit slot ranges. We can start with a more restricted command format (current proposal), and relax the rule based on the OSS community's feedback. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/* 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) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is a bit odd that "orderby" and "slotsrange" are mutually exclusive? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/* CLUSTER SLOT-STATS ORDERBY column [LIMIT limit] [ASC | DESC] */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Did we really use column before? Seems like the wrong wording for sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 on metric |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
i++; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sortAndAddReplySlotStats(c, order_by, limit, desc); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
addReplySubcommandSyntaxError(c); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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" | ||
} | ||
] | ||
} | ||
] | ||
} | ||
] | ||
} | ||
] | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember we named this because we planned to break up the APIs, but we ended up naming it weirdly (what is a "legacy cluster" when there is only one cluster) but it doesn't make a lot of sense to revert it now. I don't mind this in its own file, but maybe we call it cluster_slot_stats instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will be renamed as
cluster_slot_stats.c
instead.