Refactor the internal function parse_cluster_nodes
#119
Conversation
Parse the string without using sdssplitlen which allocates memory for each element. Parse each line once to get a valkeyClusterNode. Signed-off-by: Björn Svensson <[email protected]>
Store parsed replicas in a separate dict during parsing, and move them to their primary when all lines are parsed. This dict owns the memory for added nodes until moved. Previously the dict contained references to both replicas and primaries and a bit harder to handle. Signed-off-by: Björn Svensson <[email protected]>
There was a problem hiding this comment.
I didn't review everything very carefully, but I believe you just refactored existing logic and we have tests covering it, so it's fine by me.
We use the words master, slave, primary, replica a little bit mixed everywhere, in macros, functions, struct fieds, variables and comments. I guess we should strive towards the new terminology, but not change all in the same PR?
Use primary and replica in added code. Signed-off-by: Björn Svensson <[email protected]>
Probably less odd. Signed-off-by: Björn Svensson <[email protected]>
Signed-off-by: Björn Svensson <[email protected]>
Yes, the newly added testcases in ut_slotmap_update.c should cover this change.
The plan was to change the define and struct in a separate PR since it also affects CLUSTER SLOTS parsing, but there was some new code in this PR that was a bit inconsistent, this is now changed. |
Signed-off-by: Björn Svensson <[email protected]>
Change old way of storing replicas while parsing cluster nodes. Simply keep lists of cluster nodes in the replica dict, which then can be moved to the nodes dict. Signed-off-by: Björn Svensson <[email protected]>
| assert(primary->slaves == NULL); | ||
| /* Move replica list from replicas dict to nodes dict. */ | ||
| primary->slaves = dictGetEntryVal(der); | ||
| dictSetHashVal(replicas, der, NULL); |
There was a problem hiding this comment.
I don't recognize the function name dictSetHashVal. It must be copied from a very old Redis version. 😄
Co-authored-by: Viktor Söderqvist <[email protected]> Signed-off-by: Björn Svensson <[email protected]>
Signed-off-by: Björn Svensson <[email protected]>
This is a 2 step/2 commit refactor PR.
Refactor the internal function
parse_cluster_nodes.Parse the
CLUSTER NODESreply by a simpler string search usingstrchrand string modification instead of usingsdssplitlen, which allocates memory for each element.Parse each line once using a new function
parse_cluster_nodes_line, which returns avalkeyClusterNode.Replicas are only parsed when configured, like legacy.
Refactor the replica handling in CLUSTER NODES parsing.
Store parsed replicas in a separate dict during parsing, and move them to their primary when all lines are parsed.
This dict owns the memory for replicas until moved to the related primary node.
Previously the dict contained references to both replicas and primaries and a bit harder to handle.
The function
cluster_master_slave_mapping_with_nameis removed and its dual purpose handling is replaced by functionsstore_replica_nodeandmove_replica_nodes.