From ea72f483d475af44d2399807bc01caab919a89de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Svensson?= Date: Wed, 18 Dec 2024 10:06:08 +0100 Subject: [PATCH] Rename offensive defines and members in API (#140) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename offensive cluster role defines in the API: Use `VALKEY_ROLE_PRIMARY` instead of `VALKEY_ROLE_MASTER`. Use `VALKEY_ROLE_REPLICA` instead of `VALKEY_ROLE_SLAVE`. Use `VALKEY_ROLE_UNKNOWN` instead of `VALKEY_ROLE_NULL`. - Rename `valkeyClusterNode` member `slaves` to `replicas`. - Rename offensive variables. Signed-off-by: Björn Svensson --- docs/migration-guide.md | 7 +++ include/valkey/cluster.h | 10 ++-- src/cluster.c | 109 +++++++++++++++++++------------------- tests/ut_slotmap_update.c | 46 ++++++++-------- 4 files changed, 89 insertions(+), 83 deletions(-) diff --git a/docs/migration-guide.md b/docs/migration-guide.md index 05b77d22..1a96de44 100644 --- a/docs/migration-guide.md +++ b/docs/migration-guide.md @@ -27,10 +27,17 @@ The type `sds` is removed from the public API. * `ctx_get_by_node` is renamed to `valkeyClusterGetValkeyContext`. * `actx_get_by_node` is renamed to `valkeyClusterGetValkeyAsyncContext`. +### Renamed API defines + +* `REDIS_ROLE_NULL` is renamed to `VALKEY_ROLE_UNKNOWN`. +* `REDIS_ROLE_MASTER` is renamed to `VALKEY_ROLE_PRIMARY`. +* `REDIS_ROLE_SLAVE` is renamed to `VALKEY_ROLE_REPLICA`. + ### Removed API functions * `redisClusterSetMaxRedirect` removed and replaced with `valkeyClusterSetOptionMaxRetry`. * `redisClusterSetOptionAddNode` removed and replaced with `valkeyClusterSetOptionAddNodes`. + (Note the "s" in the end of the function name.) * `redisClusterSetOptionConnectBlock` removed since it was deprecated. * `redisClusterSetOptionConnectNonBlock` removed since it was deprecated. * `parse_cluster_nodes` removed from API, for internal use only. diff --git a/include/valkey/cluster.h b/include/valkey/cluster.h index 9a7d283d..7bb41de7 100644 --- a/include/valkey/cluster.h +++ b/include/valkey/cluster.h @@ -40,9 +40,9 @@ #define VALKEYCLUSTER_SLOTS 16384 -#define VALKEY_ROLE_NULL 0 -#define VALKEY_ROLE_MASTER 1 -#define VALKEY_ROLE_SLAVE 2 +#define VALKEY_ROLE_UNKNOWN 0 +#define VALKEY_ROLE_PRIMARY 1 +#define VALKEY_ROLE_REPLICA 2 /* Configuration flags */ #define VALKEYCLUSTER_FLAG_NULL 0x0 @@ -84,13 +84,13 @@ typedef struct valkeyClusterNode { valkeyAsyncContext *acon; int64_t lastConnectionAttempt; /* Timestamp */ struct hilist *slots; - struct hilist *slaves; + struct hilist *replicas; } valkeyClusterNode; typedef struct cluster_slot { uint32_t start; uint32_t end; - valkeyClusterNode *node; /* master that this slot region belong to */ + valkeyClusterNode *node; /* Owner of slot region. */ } cluster_slot; /* Context for accessing a Valkey Cluster */ diff --git a/src/cluster.c b/src/cluster.c index 0cefd10d..c6bd8bc8 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -275,7 +275,7 @@ static void freeValkeyClusterNode(valkeyClusterNode *node) { valkeyAsyncFree(node->acon); } listRelease(node->slots); - listRelease(node->slaves); + listRelease(node->replicas); vk_free(node); } @@ -289,7 +289,7 @@ static cluster_slot *cluster_slot_create(valkeyClusterNode *node) { slot->node = node; if (node != NULL) { - assert(node->role == VALKEY_ROLE_MASTER); + assert(node->role == VALKEY_ROLE_PRIMARY); if (node->slots == NULL) { node->slots = listCreate(); if (node->slots == NULL) { @@ -314,7 +314,7 @@ static int cluster_slot_ref_node(cluster_slot *slot, valkeyClusterNode *node) { return VALKEY_ERR; } - if (node->role != VALKEY_ROLE_MASTER) { + if (node->role != VALKEY_ROLE_PRIMARY) { return VALKEY_ERR; } @@ -424,7 +424,7 @@ static valkeyClusterNode *node_get_with_slots(valkeyClusterContext *cc, goto oom; } - if (role == VALKEY_ROLE_MASTER) { + if (role == VALKEY_ROLE_PRIMARY) { node->slots = listCreate(); if (node->slots == NULL) { goto oom; @@ -519,7 +519,7 @@ static dict *parse_cluster_slots(valkeyClusterContext *cc, valkeyReply *reply) { valkeyReply *elem_slots_begin, *elem_slots_end; valkeyReply *elem_nodes; valkeyReply *elem_ip, *elem_port; - valkeyClusterNode *master = NULL, *slave; + valkeyClusterNode *primary = NULL, *replica; uint32_t i, idx; if (reply->type != VALKEY_REPLY_ARRAY) { @@ -599,12 +599,12 @@ static dict *parse_cluster_slots(valkeyClusterContext *cc, valkeyReply *reply) { elem_port->type != VALKEY_REPLY_INTEGER) { valkeyClusterSetError(cc, VALKEY_ERR_OTHER, "Command(cluster slots) reply error: " - "master ip or port is not correct."); + "ip or port is not correct."); goto error; } - // this is master. if (idx == 2) { + /* Parse a primary node. */ sds address = sdsnewlen(elem_ip->str, elem_ip->len); if (address == NULL) { goto oom; @@ -616,11 +616,10 @@ static dict *parse_cluster_slots(valkeyClusterContext *cc, valkeyReply *reply) { den = dictFind(nodes, address); sdsfree(address); - // master already exists, break to the next slots region. if (den != NULL) { - - master = dictGetEntryVal(den); - ret = cluster_slot_ref_node(slot, master); + /* Skip parsing this primary node since it's already known. */ + primary = dictGetEntryVal(den); + ret = cluster_slot_ref_node(slot, primary); if (ret != VALKEY_OK) { goto oom; } @@ -629,50 +628,50 @@ static dict *parse_cluster_slots(valkeyClusterContext *cc, valkeyReply *reply) { break; } - master = node_get_with_slots(cc, elem_ip, elem_port, - VALKEY_ROLE_MASTER); - if (master == NULL) { + primary = node_get_with_slots(cc, elem_ip, elem_port, + VALKEY_ROLE_PRIMARY); + if (primary == NULL) { goto error; } - sds key = sdsnewlen(master->addr, sdslen(master->addr)); + sds key = sdsnewlen(primary->addr, sdslen(primary->addr)); if (key == NULL) { - freeValkeyClusterNode(master); + freeValkeyClusterNode(primary); goto oom; } - ret = dictAdd(nodes, key, master); + ret = dictAdd(nodes, key, primary); if (ret != DICT_OK) { sdsfree(key); - freeValkeyClusterNode(master); + freeValkeyClusterNode(primary); goto oom; } - ret = cluster_slot_ref_node(slot, master); + ret = cluster_slot_ref_node(slot, primary); if (ret != VALKEY_OK) { goto oom; } slot = NULL; } else if (cc->flags & VALKEYCLUSTER_FLAG_ADD_SLAVE) { - slave = node_get_with_slots(cc, elem_ip, elem_port, - VALKEY_ROLE_SLAVE); - if (slave == NULL) { + replica = node_get_with_slots(cc, elem_ip, elem_port, + VALKEY_ROLE_REPLICA); + if (replica == NULL) { goto error; } - if (master->slaves == NULL) { - master->slaves = listCreate(); - if (master->slaves == NULL) { - freeValkeyClusterNode(slave); + if (primary->replicas == NULL) { + primary->replicas = listCreate(); + if (primary->replicas == NULL) { + freeValkeyClusterNode(replica); goto oom; } - master->slaves->free = listClusterNodeDestructor; + primary->replicas->free = listClusterNodeDestructor; } - if (listAddNodeTail(master->slaves, slave) == NULL) { - freeValkeyClusterNode(slave); + if (listAddNodeTail(primary->replicas, replica) == NULL) { + freeValkeyClusterNode(replica); goto oom; } } @@ -740,9 +739,9 @@ static int store_replica_nodes(dict *nodes, dict *replicas) { /* Move replica nodes related to this primary. */ dictEntry *der = dictFind(replicas, primary->name); if (der != NULL) { - assert(primary->slaves == NULL); + assert(primary->replicas == NULL); /* Move replica list from replicas dict to nodes dict. */ - primary->slaves = dictGetEntryVal(der); + primary->replicas = dictGetEntryVal(der); dictSetHashVal(replicas, der, NULL); } } @@ -785,29 +784,29 @@ static int parse_cluster_nodes_line(valkeyClusterContext *cc, char *line, /* Parse flags, a comma separated list of following flags: * myself, master, slave, fail?, fail, handshake, noaddr, nofailover, noflags. */ - uint8_t role = VALKEY_ROLE_NULL; + uint8_t role = VALKEY_ROLE_UNKNOWN; while (*flags != '\0') { if ((p = strchr(flags, ',')) != NULL) *p = '\0'; if (memcmp(flags, "master", 6) == 0) { - role = VALKEY_ROLE_MASTER; + role = VALKEY_ROLE_PRIMARY; break; } if (memcmp(flags, "slave", 5) == 0) { - role = VALKEY_ROLE_SLAVE; + role = VALKEY_ROLE_REPLICA; break; } if (p == NULL) /* No more flags. */ break; flags = p + 1; /* Start of next flag. */ } - if (role == VALKEY_ROLE_NULL) { + if (role == VALKEY_ROLE_UNKNOWN) { valkeyClusterSetError(cc, VALKEY_ERR_OTHER, "Unknown role"); return VALKEY_ERR; } /* Only parse replicas when requested. */ - if (role == VALKEY_ROLE_SLAVE && parsed_primary_id == NULL) { + if (role == VALKEY_ROLE_REPLICA && parsed_primary_id == NULL) { *parsed_node = NULL; return VALKEY_OK; } @@ -852,8 +851,8 @@ static int parse_cluster_nodes_line(valkeyClusterContext *cc, char *line, p++; // Skip separator character. node->port = vk_atoi(p, strlen(p)); - /* No slot parsing needed for replicas, but return master id. */ - if (node->role == VALKEY_ROLE_SLAVE) { + /* No slot parsing needed for replicas, but return primary id. */ + if (node->role == VALKEY_ROLE_REPLICA) { *parsed_primary_id = primary_id; *parsed_node = node; return VALKEY_OK; @@ -941,7 +940,7 @@ static dict *parse_cluster_nodes(valkeyClusterContext *cc, valkeyReply *reply) { goto error; if (node == NULL) continue; /* Line skipped. */ - if (node->role == VALKEY_ROLE_MASTER) { + if (node->role == VALKEY_ROLE_PRIMARY) { sds key = sdsnew(node->addr); if (key == NULL) { freeValkeyClusterNode(node); @@ -962,7 +961,7 @@ static dict *parse_cluster_nodes(valkeyClusterContext *cc, valkeyReply *reply) { slot_ranges_found += listLength(node->slots); } else { - assert(node->role == VALKEY_ROLE_SLAVE); + assert(node->role == VALKEY_ROLE_REPLICA); if (replicas == NULL) { if ((replicas = dictCreate(&clusterNodeListDictType, NULL)) == NULL) { freeValkeyClusterNode(node); @@ -1124,19 +1123,19 @@ static int updateNodesAndSlotmap(valkeyClusterContext *cc, dict *nodes) { dictEntry *de; while ((de = dictNext(&di))) { - valkeyClusterNode *master = dictGetEntryVal(de); - if (master->role != VALKEY_ROLE_MASTER) { + valkeyClusterNode *node = dictGetEntryVal(de); + if (node->role != VALKEY_ROLE_PRIMARY) { valkeyClusterSetError(cc, VALKEY_ERR_OTHER, - "Node role must be master"); + "Node role must be primary"); goto error; } - if (master->slots == NULL) { + if (node->slots == NULL) { continue; } listIter li; - listRewind(master->slots, &li); + listRewind(node->slots, &li); listNode *ln; while ((ln = listNext(&li))) { @@ -1152,7 +1151,7 @@ static int updateNodesAndSlotmap(valkeyClusterContext *cc, dict *nodes) { "Different node holds same slot"); goto error; } - table[i] = master; + table[i] = node; } } } @@ -1590,20 +1589,20 @@ int valkeyClusterSetOptionTimeout(valkeyClusterContext *cc, valkeySetTimeout(node->con, tv); } - if (node->slaves && listLength(node->slaves) > 0) { - valkeyClusterNode *slave; + if (node->replicas && listLength(node->replicas) > 0) { + valkeyClusterNode *replica; listNode *ln; listIter li; - listRewind(node->slaves, &li); + listRewind(node->replicas, &li); while ((ln = listNext(&li)) != NULL) { - slave = listNodeValue(ln); - if (slave->acon) { - valkeyAsyncSetTimeout(slave->acon, tv); + replica = listNodeValue(ln); + if (replica->acon) { + valkeyAsyncSetTimeout(replica->acon, tv); } - if (slave->con && slave->con->err == 0) { - valkeySetTimeout(slave->con, tv); + if (replica->con && replica->con->err == 0) { + valkeySetTimeout(replica->con, tv); } } } @@ -1861,7 +1860,7 @@ static valkeyClusterNode *getNodeFromRedirectReply(valkeyClusterContext *cc, if (node == NULL) { goto oom; } - node->role = VALKEY_ROLE_MASTER; + node->role = VALKEY_ROLE_PRIMARY; node->addr = part[2]; part[2] = NULL; /* Memory ownership moved */ diff --git a/tests/ut_slotmap_update.c b/tests/ut_slotmap_update.c index 5d80f12c..bfeec6e5 100644 --- a/tests/ut_slotmap_update.c +++ b/tests/ut_slotmap_update.c @@ -63,18 +63,18 @@ void test_parse_cluster_nodes(bool parse_replicas) { assert(strcmp(node->addr, "127.0.0.1:30001") == 0); assert(strcmp(node->host, "127.0.0.1") == 0); assert(node->port == 30001); - assert(node->role == VALKEY_ROLE_MASTER); + assert(node->role == VALKEY_ROLE_PRIMARY); assert(listLength(node->slots) == 1); /* 1 slot range */ slot = listNodeValue(listFirst(node->slots)); assert(slot->start == 0); assert(slot->end == 5460); if (parse_replicas) { - assert(listLength(node->slaves) == 1); - node = listNodeValue(listFirst(node->slaves)); + assert(listLength(node->replicas) == 1); + node = listNodeValue(listFirst(node->replicas)); assert(strcmp(node->name, "07c37dfeb235213a872192d90877d0cd55635b91") == 0); - assert(node->role == VALKEY_ROLE_SLAVE); + assert(node->role == VALKEY_ROLE_REPLICA); } else { - assert(node->slaves == NULL); + assert(node->replicas == NULL); } /* Verify node 2 */ node = dictGetEntryVal(dictNext(&di)); @@ -82,18 +82,18 @@ void test_parse_cluster_nodes(bool parse_replicas) { assert(strcmp(node->addr, "127.0.0.1:30002") == 0); assert(strcmp(node->host, "127.0.0.1") == 0); assert(node->port == 30002); - assert(node->role == VALKEY_ROLE_MASTER); + assert(node->role == VALKEY_ROLE_PRIMARY); assert(listLength(node->slots) == 1); /* 1 slot range */ slot = listNodeValue(listFirst(node->slots)); assert(slot->start == 5461); assert(slot->end == 10922); if (parse_replicas) { - assert(listLength(node->slaves) == 1); - node = listNodeValue(listFirst(node->slaves)); + assert(listLength(node->replicas) == 1); + node = listNodeValue(listFirst(node->replicas)); assert(strcmp(node->name, "6ec23923021cf3ffec47632106199cb7f496ce01") == 0); - assert(node->role == VALKEY_ROLE_SLAVE); + assert(node->role == VALKEY_ROLE_REPLICA); } else { - assert(node->slaves == NULL); + assert(node->replicas == NULL); } /* Verify node 3 */ node = dictGetEntryVal(dictNext(&di)); @@ -101,18 +101,18 @@ void test_parse_cluster_nodes(bool parse_replicas) { assert(strcmp(node->addr, "127.0.0.1:30003") == 0); assert(strcmp(node->host, "127.0.0.1") == 0); assert(node->port == 30003); - assert(node->role == VALKEY_ROLE_MASTER); + assert(node->role == VALKEY_ROLE_PRIMARY); assert(listLength(node->slots) == 1); /* 1 slot range */ slot = listNodeValue(listFirst(node->slots)); assert(slot->start == 10923); assert(slot->end == 16383); if (parse_replicas) { - assert(listLength(node->slaves) == 1); - node = listNodeValue(listFirst(node->slaves)); + assert(listLength(node->replicas) == 1); + node = listNodeValue(listFirst(node->replicas)); assert(strcmp(node->name, "824fe116063bc5fcf9f4ffd895bc17aee7731ac3") == 0); - assert(node->role == VALKEY_ROLE_SLAVE); + assert(node->role == VALKEY_ROLE_REPLICA); } else { - assert(node->slaves == NULL); + assert(node->replicas == NULL); } dictRelease(nodes); @@ -275,35 +275,35 @@ void test_parse_cluster_nodes_with_multiple_replicas(void) { assert(strcmp(node->addr, "127.0.0.1:30001") == 0); assert(strcmp(node->host, "127.0.0.1") == 0); assert(node->port == 30001); - assert(node->role == VALKEY_ROLE_MASTER); + assert(node->role == VALKEY_ROLE_PRIMARY); assert(listLength(node->slots) == 1); /* 1 slot range */ slot = listNodeValue(listFirst(node->slots)); assert(slot->start == 0); assert(slot->end == 16383); /* Verify replicas. */ - assert(listLength(node->slaves) == 5); - listRewind(node->slaves, &li); + assert(listLength(node->replicas) == 5); + listRewind(node->replicas, &li); node = listNodeValue(listNext(&li)); assert(strcmp(node->name, "07c37dfeb235213a872192d90877d0cd55635b91") == 0); assert(strcmp(node->addr, "127.0.0.1:30004") == 0); - assert(node->role == VALKEY_ROLE_SLAVE); + assert(node->role == VALKEY_ROLE_REPLICA); node = listNodeValue(listNext(&li)); assert(strcmp(node->name, "6ec23923021cf3ffec47632106199cb7f496ce01") == 0); assert(strcmp(node->addr, "127.0.0.1:30005") == 0); - assert(node->role == VALKEY_ROLE_SLAVE); + assert(node->role == VALKEY_ROLE_REPLICA); node = listNodeValue(listNext(&li)); assert(strcmp(node->name, "824fe116063bc5fcf9f4ffd895bc17aee7731ac3") == 0); assert(strcmp(node->addr, "127.0.0.1:30006") == 0); - assert(node->role == VALKEY_ROLE_SLAVE); + assert(node->role == VALKEY_ROLE_REPLICA); node = listNodeValue(listNext(&li)); assert(strcmp(node->name, "67ed2db8d677e59ec4a4cefb06858cf2a1a89fa1") == 0); assert(strcmp(node->addr, "127.0.0.1:30002") == 0); - assert(node->role == VALKEY_ROLE_SLAVE); + assert(node->role == VALKEY_ROLE_REPLICA); node = listNodeValue(listNext(&li)); assert(strcmp(node->name, "292f8b365bb7edb5e285caf0b7e6ddc7265d2f4f") == 0); assert(strcmp(node->addr, "127.0.0.1:30003") == 0); - assert(node->role == VALKEY_ROLE_SLAVE); + assert(node->role == VALKEY_ROLE_REPLICA); dictRelease(nodes); valkeyClusterFree(cc);