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

["PragmaTwice (via GitHub)" <gi...@apache.org>]

PragmaTwice commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1182072791


##########
src/common/encoding.h:
##########
@@ -23,6 +23,7 @@
 #include <rocksdb/slice.h>
 #include <unistd.h>
 
+#include <cstdint>

Review Comment:
   ```suggestion
   ```



##########
x.py:
##########
@@ -106,7 +106,7 @@ def build(dir: str, jobs: Optional[int], ghproxy: bool, ninja: bool, unittest: b
 
     makedirs(dir, exist_ok=True)
 
-    cmake_options = ["-DCMAKE_BUILD_TYPE=RelWithDebInfo"]
+    cmake_options = []

Review Comment:
   related to the change to CMakeLists.txt



##########
src/cluster/cluster.cc:
##########
@@ -128,30 +122,43 @@ Status Cluster::SetSlot(int slot, const std::string &node_id, int64_t new_versio
     return {Status::NotOK, errNoMasterNode};
   }
 
+  std::vector<std::pair<int, int>> slots;
+  Status s = parseSlotRanges(slots_str, slots);
+  if (!s.OK()) {
+    return s;
+  }
+
   // Update version
   version_ = new_version;
 
   // Update topology
   //  1. Remove the slot from old node if existing
   //  2. Add the slot into to-assign node
   //  3. Update the map of slots to nodes.
-  std::shared_ptr<ClusterNode> old_node = slots_nodes_[slot];
-  if (old_node != nullptr) {
-    old_node->slots[slot] = false;
-  }
-  to_assign_node->slots[slot] = true;
-  slots_nodes_[slot] = to_assign_node;
-
-  // Clear data of migrated slot or record of imported slot
-  if (old_node == myself_ && old_node != to_assign_node) {
-    // If slot is migrated from this node
-    if (migrated_slots_.count(slot) > 0) {
-      svr_->slot_migrator->ClearKeysOfSlot(kDefaultNamespace, slot);
-      migrated_slots_.erase(slot);
-    }
-    // If slot is imported into this node
-    if (imported_slots_.count(slot) > 0) {
-      imported_slots_.erase(slot);
+  // remember: The atomicity of the process is based on
+  // the transactionality of ClearKeysOfSlot().
+  for (const auto &slot_range : slots) {
+    int s_start = slot_range.first, s_end = slot_range.second;

Review Comment:
   ```suggestion
     for (auto [s_start, s_end] : slots) {
   ```



##########
CMakeLists.txt:
##########
@@ -164,6 +166,12 @@ if ((PROJECT_VERSION STREQUAL "unstable") AND (GIT_SHA STREQUAL ""))
 endif ()
 configure_file(src/version.h.in ${PROJECT_BINARY_DIR}/version.h)
 
+if (NOT CMAKE_BUILD_TYPE OR CMAKE_BUILD_TYPE STREQUAL "None")
+    set (CMAKE_BUILD_TYPE "RelWithDebInfo")
+    message (STATUS "CMAKE_BUILD_TYPE is not set, set to default = ${CMAKE_BUILD_TYPE}")
+endif ()
+message (STATUS "CMAKE_BUILD_TYPE: ${CMAKE_BUILD_TYPE}")

Review Comment:
   I think change the value of `CMAKE_BUILD_TYPE` after the `project()` command (and even after all dependency fetching and configuration here) can be a totally mess (the dependency and many target can be mis-configured). 



##########
src/cluster/cluster.cc:
##########
@@ -641,6 +648,56 @@ 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)};
+  }
+
+  auto valid_range = NumericRange<int>{0, kClusterSlots - 1};
+  // Parse all slots(include slot ranges)
+  for (auto &slot_range : slot_ranges) {
+    if (slot_range.find('-') == std::string::npos) {
+      auto parse_result = ParseInt<int>(slot_range, valid_range, 10);
+      if (!parse_result) {
+        return {Status::NotOK, errInvalidSlotID};
+      }
+      int slot_start = *parse_result;
+      slots.emplace_back(std::make_pair(slot_start, slot_start));
+      continue;
+    }
+
+    // parse slot range: "int1-int2" (satisfy: int1 <= int2 )
+    if (slot_range.back() == '-') {
+      return {Status::NotOK,
+              fmt::format("Invalid slot range: {}. The character '-' can't appear in the last position.", slot_range)};
+    }

Review Comment:
   It seems to be unnecessary since the code below (split according to `-` and expect `size() == 2`) already did this check.



##########
CMakeLists.txt:
##########
@@ -33,6 +33,8 @@ option(PORTABLE "build a portable binary (disable arch-specific optimizations)"
 # TODO: set ENABLE_NEW_ENCODING to ON when we are ready
 option(ENABLE_NEW_ENCODING "enable new encoding (#1033) for storing 64bit size and expire time in milliseconds" OFF)
 
+set(CMAKE_LINK_DEPENDS_NO_SHARED 1)

Review Comment:
   ```suggestion
   set(CMAKE_LINK_DEPENDS_NO_SHARED ON)
   ```
   
   And, could you share some reason for this change?



##########
src/cluster/cluster.cc:
##########
@@ -641,6 +648,56 @@ 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)};
+  }
+
+  auto valid_range = NumericRange<int>{0, kClusterSlots - 1};
+  // Parse all slots(include slot ranges)
+  for (auto &slot_range : slot_ranges) {
+    if (slot_range.find('-') == std::string::npos) {
+      auto parse_result = ParseInt<int>(slot_range, valid_range, 10);
+      if (!parse_result) {
+        return {Status::NotOK, errInvalidSlotID};
+      }
+      int slot_start = *parse_result;
+      slots.emplace_back(std::make_pair(slot_start, slot_start));
+      continue;
+    }
+
+    // parse slot range: "int1-int2" (satisfy: int1 <= int2 )
+    if (slot_range.back() == '-') {
+      return {Status::NotOK,
+              fmt::format("Invalid slot range: {}. The character '-' can't appear in the last position.", slot_range)};
+    }
+    std::vector<std::string> fields = util::Split(slot_range, "-");
+    if (fields.size() != 2) {
+      return {Status::NotOK,
+              fmt::format("Invalid slot range: {}. The slot range should be of the form `int1-int2`.", slot_range)};
+    }
+    auto parse_start = ParseInt<int>(fields[0], valid_range, 10);
+    auto parse_end = ParseInt<int>(fields[1], valid_range, 10);
+    if (!parse_start || !parse_end || *parse_start > *parse_end) {
+      return {Status::NotOK,
+              fmt::format(
+                  "Invalid slot range: {}. The slot range `int1-int2` needs to satisfy the condition (int1 <= int2).",
+                  slot_range)};
+    }
+    int slot_start = *parse_start;
+    int slot_end = *parse_end;
+    slots.emplace_back(std::make_pair(slot_start, slot_end));

Review Comment:
   ```suggestion
       slots.emplace_back(*parse_start, *parse_end);
   ```



##########
src/cluster/redis_slot.h:
##########
@@ -19,6 +19,7 @@
  */
 
 #pragma once
+#include <cstdint>

Review Comment:
   ```suggestion
   ```



##########
src/cluster/cluster.cc:
##########
@@ -641,6 +648,56 @@ 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)};
+  }
+
+  auto valid_range = NumericRange<int>{0, kClusterSlots - 1};
+  // Parse all slots(include slot ranges)

Review Comment:
   ```suggestion
     // Parse all slots (include slot ranges)
   ```



-- 
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