[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT

["git-hulk (via GitHub)" <gi...@apache.org>]

git-hulk commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1183222751


##########
src/cluster/cluster.cc:
##########
@@ -641,6 +647,53 @@ Status Cluster::LoadClusterNodes(const std::string &file_path) {
   return SetClusterNodes(nodes_info, version, false);
 }
 
+// TODO: maybe it needs to use a more precise error type to represent `NotOk`.
+Status Cluster::parseSlotRanges(const std::string &slots_str, std::vector<std::pair<int, int>> &slots) {
+  if (slots_str.empty()) {
+    return {Status::NotOK, "No slots to parse."};
+  }
+  std::vector<std::string> slot_ranges = util::Split(slots_str, " ");
+
+  if (slot_ranges.empty()) {
+    return {Status::NotOK, fmt::format("Invalid slots: {}. Please use ' ' to space slots.", slots_str)};

Review Comment:
   ```suggestion
       return {Status::NotOK, fmt::format("Invalid slots: {}. Please use spaces to separate slots.", slots_str)};
   ```



##########
src/cluster/cluster.cc:
##########
@@ -641,6 +647,53 @@ Status Cluster::LoadClusterNodes(const std::string &file_path) {
   return SetClusterNodes(nodes_info, version, false);
 }
 
+// TODO: maybe it needs to use a more precise error type to represent `NotOk`.
+Status Cluster::parseSlotRanges(const std::string &slots_str, std::vector<std::pair<int, int>> &slots) {

Review Comment:
   Could you add some test cases for parseSlotRanges?



##########
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.
 // 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) {
+  // the parsing process takes some time, so let's check other arguments first.

Review Comment:
   Can remove meaningless comment line
   ```suggestion
   ```



-- 
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: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org