caipengbo commented on code in PR #1414:
URL:
https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1183631124
##########
src/cluster/cluster.cc:
##########
@@ -93,32 +93,26 @@ Status Cluster::SetNodeId(const std::string &node_id) {
return Status::OK();
}
-// Set the slot to the node if new version is current version +1. It is useful
-// when we scale cluster avoid too many big messages, since we only update one
-// slot distribution and there are 16384 slot in our design.
-//
+// Set the slots to the node if new version is current version +1. It is useful
+// when we scale cluster avoid too many big messages. By th way,there are
16384 slots
+// in our design.
// The reason why the new version MUST be +1 of current version is that,
// the command changes topology based on specific topology (also means specific
// version), we must guarantee current topology is exactly expected, otherwise,
// this update may make topology corrupt, so base topology version is very
important.
// This is different with CLUSTERX SETNODES commands because it uses new
version
// topology to cover current version, it allows kvrocks nodes lost some
topology
// updates since of network failure, it is state instead of operation.
-Status Cluster::SetSlot(int slot, const std::string &node_id, int64_t
new_version) {
- // Parameters check
+Status Cluster::SetSlot(const std::string &slots_str, const std::string
&node_id, int64_t new_version) {
Review Comment:
Just pass in the valid slotrange and hand the parsing and validation to the
outer layer. You can rename this function to `SetSlots` or `SetSlotRanges`.
##########
src/commands/cmd_cluster.cc:
##########
@@ -219,7 +210,7 @@ class CommandClusterX : public Commander {
*output = redis::Error(s.Msg());
}
} else if (subcommand_ == "setslot") {
- Status s = svr->cluster->SetSlot(slot_id_, args_[4], set_version_);
+ Status s = svr->cluster->SetSlot(slots_str_, args_[4], set_version_);
Review Comment:
I also think it would be better to do this in command parsing. Only valid
slotranges is concerned in `cluster.cc` and these parsing aspects are not
concerned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
