From 546cef6684e02bf22aeed9a5fca2a685babfe465 Mon Sep 17 00:00:00 2001 From: Madelyn Olson Date: Tue, 14 May 2024 17:09:49 -0700 Subject: [PATCH] Initial cleanup for cluster refactoring (#460) Cleaned up the minor cluster refactoring notes that were intended to be follow ups that never happened. Basically: 1. Minor style nitpicks 2. Generalized clusterNodeIsMyself so that it wasn't implementation dependent. 3. Removed getMyClusterId, and just make it an explicit call to myself's name, which seems more straightforward and removes unnecessary abstraction. 4. Remove clusterNodeGetSlaveof infavor of clusterNodeGetMaster. We already do a check if it's a replica, and if it wasn't working it would have been crashing. Signed-off-by: Madelyn Olson --- src/cluster.c | 8 ++++---- src/cluster.h | 11 ++++------- src/cluster_legacy.c | 10 +--------- src/module.c | 6 +++--- 4 files changed, 12 insertions(+), 23 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index 99c02cd86..741b05c60 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -789,8 +789,8 @@ void clusterCommandMyId(client *c) { } } -char* getMyClusterId(void) { - return clusterNodeGetName(getMyClusterNode()); +int clusterNodeIsMyself(clusterNode *n) { + return n == getMyClusterNode(); } void clusterCommandMyShardId(client *c) { @@ -1193,7 +1193,7 @@ clusterNode *getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, i if (((c->flags & CLIENT_READONLY) || pubsubshard_included) && !is_write_command && clusterNodeIsSlave(myself) && - clusterNodeGetSlaveof(myself) == n) + clusterNodeGetMaster(myself) == n) { return myself; } @@ -1286,7 +1286,7 @@ int clusterRedirectBlockedClientIfNeeded(client *c) { * replica can handle, allow it. */ if ((c->flags & CLIENT_READONLY) && !(c->lastcmd->flags & CMD_WRITE) && - clusterNodeIsSlave(myself) && clusterNodeGetSlaveof(myself) == node) + clusterNodeIsSlave(myself) && clusterNodeGetMaster(myself) == node) { node = myself; } diff --git a/src/cluster.h b/src/cluster.h index 8a2b97cad..481fcc973 100644 --- a/src/cluster.h +++ b/src/cluster.h @@ -60,13 +60,13 @@ int handleDebugClusterCommand(client *c); const char **clusterDebugCommandExtendedHelp(void); /* handle implementation specific cluster commands. Return 1 if handled, 0 otherwise. */ int clusterCommandSpecial(client *c); -const char** clusterCommandExtendedHelp(void); +const char **clusterCommandExtendedHelp(void); int clusterAllowFailoverCmd(client *c); void clusterPromoteSelfToMaster(void); int clusterManualFailoverTimeLimit(void); -void clusterCommandSlots(client * c); +void clusterCommandSlots(client *c); void clusterCommandMyId(client *c); void clusterCommandMyShardId(client *c); void clusterCommandShards(client *c); @@ -74,19 +74,15 @@ sds clusterGenNodeDescription(client *c, clusterNode *node, int tls_primary); int clusterNodeCoversSlot(clusterNode *n, int slot); int getNodeDefaultClientPort(clusterNode *n); -int clusterNodeIsMyself(clusterNode *n); clusterNode *getMyClusterNode(void); -char *getMyClusterId(void); int getClusterSize(void); int getMyShardSlotCount(void); int handleDebugClusterCommand(client *c); -int clusterNodePending(clusterNode *node); +int clusterNodePending(clusterNode *node); int clusterNodeIsMaster(clusterNode *n); char **getClusterNodesList(size_t *numnodes); -int clusterNodeIsMaster(clusterNode *n); char *clusterNodeIp(clusterNode *node); int clusterNodeIsSlave(clusterNode *node); -clusterNode *clusterNodeGetSlaveof(clusterNode *node); clusterNode *clusterNodeGetMaster(clusterNode *node); char *clusterNodeGetName(clusterNode *node); int clusterNodeTimedOut(clusterNode *node); @@ -106,6 +102,7 @@ clusterNode *clusterLookupNode(const char *name, int length); void clusterReplicateOpenSlots(void); /* functions with shared implementations */ +int clusterNodeIsMyself(clusterNode *n); clusterNode *getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, int argc, int *hashslot, int *ask); int clusterRedirectBlockedClientIfNeeded(client *c); void clusterRedirectClient(client *c, clusterNode *n, int hashslot, int error_code); diff --git a/src/cluster_legacy.c b/src/cluster_legacy.c index a50d0f845..b2e9690fd 100644 --- a/src/cluster_legacy.c +++ b/src/cluster_legacy.c @@ -6090,10 +6090,6 @@ unsigned int countChannelsInSlot(unsigned int hashslot) { return kvstoreDictSize(server.pubsubshard_channels, hashslot); } -int clusterNodeIsMyself(clusterNode *n) { - return n == server.cluster->myself; -} - clusterNode *getMyClusterNode(void) { return server.cluster->myself; } @@ -6175,7 +6171,7 @@ int handleDebugClusterCommand(client *c) { return 1; } -int clusterNodePending(clusterNode *node) { +int clusterNodePending(clusterNode *node) { return node->flags & (CLUSTER_NODE_NOADDR|CLUSTER_NODE_HANDSHAKE); } @@ -6187,10 +6183,6 @@ int clusterNodeIsSlave(clusterNode *node) { return node->flags & CLUSTER_NODE_SLAVE; } -clusterNode *clusterNodeGetSlaveof(clusterNode *node) { - return node->slaveof; -} - clusterNode *clusterNodeGetMaster(clusterNode *node) { while (node->slaveof != NULL) node = node->slaveof; return node; diff --git a/src/module.c b/src/module.c index 01abd7adf..1d510461a 100644 --- a/src/module.c +++ b/src/module.c @@ -9017,7 +9017,7 @@ void VM_FreeClusterNodesList(char **ids) { * is disabled. */ const char *VM_GetMyClusterID(void) { if (!server.cluster_enabled) return NULL; - return getMyClusterId(); + return clusterNodeGetName(getMyClusterNode()); } /* Return the number of nodes in the cluster, regardless of their state @@ -9064,8 +9064,8 @@ int VM_GetClusterNodeInfo(ValkeyModuleCtx *ctx, const char *id, char *ip, char * /* If the information is not available, the function will set the * field to zero bytes, so that when the field can't be populated the * function kinda remains predictable. */ - if (clusterNodeIsSlave(node) && clusterNodeGetSlaveof(node)) - memcpy(master_id, clusterNodeGetName(clusterNodeGetSlaveof(node)) ,VALKEYMODULE_NODE_ID_LEN); + if (clusterNodeIsSlave(node) && clusterNodeGetMaster(node)) + memcpy(master_id, clusterNodeGetName(clusterNodeGetMaster(node)) ,VALKEYMODULE_NODE_ID_LEN); else memset(master_id,0,VALKEYMODULE_NODE_ID_LEN); }