You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "infdahai (via GitHub)" <gi...@apache.org> on 2023/05/01 18:31:06 UTC
[GitHub] [incubator-kvrocks] infdahai opened a new pull request, #1414: support for cluster slot batch
infdahai opened a new pull request, #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414
closes #1412
--
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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1182151137
##########
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:
just avoid relinking all depended targets on .so. It't necessary now, so I delete it.
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#issuecomment-1535799713
LGTM, I think we can remove TODO comments, then submit an issue if you feel it's necessary.
--
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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1182174260
##########
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:
in util/Split(),this process is below.
```cpp
size_t begin = 0, end = in.find_first_of(delim);
do {
std::string elem = in.substr(begin, end - begin);
if (!elem.empty()) out.push_back(std::move(elem));
if (end == std::string::npos) break;
begin = end + 1;
end = in.find_first_of(delim, begin);
} while (true);
```
when I use the range such as `3-4-`, I also get a valid result. Because the last `-` just ignores and breaks in this func.
--
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
[GitHub] [incubator-kvrocks] infdahai commented on pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#issuecomment-1531063939
> CI seems still blocked in format. Not sure your clang-format version, but you can try clang-format 12, which is used in CI.
>
> https://github.com/llvm/llvm-project/releases/tag/llvmorg-12.0.1
>
> And I can confirm that clang-format 14 on my local has also no issue.
>
> ```
> /home/runner/work/incubator-kvrocks/incubator-kvrocks/src/cluster/cluster.cc:474:39: error: code should be clang-formatted [-Wclang-format-violations]
> for (const auto &id : n->replicas) { // replicas
> ^
> ```
Sorry for this problem. My llvm version is not unified and the `clang-format` version is 16.
you're welcome. Ignore this problem.
--
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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1182004201
##########
src/cluster/cluster.cc:
##########
@@ -641,6 +649,54 @@ 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::parseSlotRange(const std::string &slots_str, std::vector<std::pair<int, int>> &slots) {
+ if (slots_str.empty()) {
+ return {Status::NotOK, "Don't use empty slots."};
+ }
+ 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 is_number = [&](const std::string &s) {
+#if __cplusplus >= 202002L
+ return std::ranges::all_of(s.begin(), s.end(), [](char c) { return isdigit(c) != 0; });
+#else
+ for (char c : s) {
+ if (isdigit(c) == 0) {
+ return false;
+ }
+ }
+ return true;
+#endif
+ };
+
+ // Parse all slots(include slot ranges)
+ for (const auto &slot_range : slot_ranges) {
+ if (is_number(slot_range)) {
Review Comment:
yeah, I change the logic.
##########
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'm not sure to do this. `CommandClusterX::Parse` is only used to find some keywords and do the simple check.
If necessary, I can migrate `parse` functions in `cluster.h` to `CommandClusterX`.
##########
src/cluster/cluster.cc:
##########
@@ -641,6 +649,54 @@ 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::parseSlotRange(const std::string &slots_str, std::vector<std::pair<int, int>> &slots) {
+ if (slots_str.empty()) {
+ return {Status::NotOK, "Don't use empty slots."};
+ }
+ 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 is_number = [&](const std::string &s) {
+#if __cplusplus >= 202002L
+ return std::ranges::all_of(s.begin(), s.end(), [](char c) { return isdigit(c) != 0; });
+#else
+ for (char c : s) {
+ if (isdigit(c) == 0) {
+ return false;
+ }
+ }
+ return true;
+#endif
+ };
+
+ // Parse all slots(include slot ranges)
+ for (const auto &slot_range : slot_ranges) {
+ if (is_number(slot_range)) {
+ int s_start = stoi(slot_range);
+ assert(IsValidSlot(s_start));
+ slots.emplace_back(std::make_pair(s_start, s_start));
+ continue;
+ }
+
+ // parse slot range: "int1-int2" (satisfy: int1 <= int2 )
+ assert(slot_range.back() != '-');
+ std::vector<std::string> fields = util::Split(slot_range, "-");
+ assert(fields.size() == 2);
Review Comment:
you're right. I will remove `assert`.
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "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_r1185744488
##########
src/cluster/cluster.cc:
##########
@@ -93,32 +84,27 @@ 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.
Review Comment:
```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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1185743984
##########
src/cluster/cluster_defs.h:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include "cluster/redis_slot.h"
+
+enum {
+ kClusterMaster = 1,
+ kClusterSlave = 2,
+ kClusterNodeIdLen = 40,
+ kClusterPortIncr = 10000,
+ kClusterSlots = HASH_SLOTS_SIZE,
+};
+
+inline constexpr const char *errInvalidNodeID = "Invalid cluster node id";
+inline constexpr const char *errInvalidSlotID = "Invalid slot id";
+inline constexpr const char *errSlotOutOfRange = "Slot is out of range";
+inline constexpr const char *errInvalidClusterVersion = "Invalid cluster version";
+inline constexpr const char *errSlotOverlapped = "Slot distribution is overlapped";
+inline constexpr const char *errNoMasterNode = "The node isn't a master";
+inline constexpr const char *errClusterNoInitialized = "CLUSTERDOWN The cluster is not initialized";
+inline constexpr const char *errInvalidClusterNodeInfo = "Invalid cluster nodes info";
+inline constexpr const char *errInvalidImportState = "Invalid import state";
+
+using SlotRange = std::pair<int, int>;
Review Comment:
I add "\n" to the line in local env but It doesn't seem to see it in github. If I add two `\n` with increasing two line nums and then I get the above situation, but `clang-format` will fix this.
for example,
https://github.com/apache/incubator-kvrocks/blob/30e9fd788a4acb585566a008f4d5fff65ee06ce3/src/cluster/slot_import.h#L53-L59
--
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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1182184410
##########
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:
yes, you are right. I just want to make `debug`,so shall I do this check in `x.py build`?
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1182196304
##########
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:
IIRC `x.py` already has a default build type.
--
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
[GitHub] [incubator-kvrocks] caipengbo commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1185734066
##########
src/cluster/cluster_defs.h:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include "cluster/redis_slot.h"
+
+enum {
Review Comment:
I move the enum struct and strings defined in `cluster` class.
It reduce dependencies and we can add this header in another module, such as `commander.h`.
Perhaps we can move many strings like this if we want to use these in other classs.
##########
src/cluster/cluster.cc:
##########
@@ -135,23 +120,29 @@ Status Cluster::SetSlot(int slot, const std::string &node_id, int64_t new_versio
// 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 (auto [s_start, s_end] : slot_ranges) {
+ for (int slot = s_start; slot <= s_end; slot++) {
+ 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);
Review Comment:
One `ClearKeyOfSlot` call one writebatch of rocksdb.
So this process exists consistency issues if `slot` in the middle is down.
I don't know what to do with it yet
--
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
[GitHub] [incubator-kvrocks] PragmaTwice closed pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice closed pull request #1414: support slot batch for CLUSTERX SETSLOT
URL: https://github.com/apache/incubator-kvrocks/pull/1414
--
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
[GitHub] [incubator-kvrocks] git-hulk merged pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414
--
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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1182003977
##########
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'm not sure to do this. `CommandClusterX::Parse` is only used to find some keywords and do the simple check now.
If necessary, I can migrate `parse` functions in `cluster.h` to `CommandClusterX`.
--
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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1182165510
##########
src/cluster/redis_slot.h:
##########
@@ -19,6 +19,7 @@
*/
#pragma once
+#include <cstdint>
Review Comment:
Please see the result in https://godbolt.org/z/5q6zj5W9s . So I think the header is due.
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#issuecomment-1546881888
Thanks all, merging...
--
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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1185743984
##########
src/cluster/cluster_defs.h:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include "cluster/redis_slot.h"
+
+enum {
+ kClusterMaster = 1,
+ kClusterSlave = 2,
+ kClusterNodeIdLen = 40,
+ kClusterPortIncr = 10000,
+ kClusterSlots = HASH_SLOTS_SIZE,
+};
+
+inline constexpr const char *errInvalidNodeID = "Invalid cluster node id";
+inline constexpr const char *errInvalidSlotID = "Invalid slot id";
+inline constexpr const char *errSlotOutOfRange = "Slot is out of range";
+inline constexpr const char *errInvalidClusterVersion = "Invalid cluster version";
+inline constexpr const char *errSlotOverlapped = "Slot distribution is overlapped";
+inline constexpr const char *errNoMasterNode = "The node isn't a master";
+inline constexpr const char *errClusterNoInitialized = "CLUSTERDOWN The cluster is not initialized";
+inline constexpr const char *errInvalidClusterNodeInfo = "Invalid cluster nodes info";
+inline constexpr const char *errInvalidImportState = "Invalid import state";
+
+using SlotRange = std::pair<int, int>;
Review Comment:
I add "\n" to the line in local env but it doesn't seem to see it in github. If I add two `\n` with increasing two line nums and then I get the above situation, but `clang-format` will fix this.
for example,
https://github.com/apache/incubator-kvrocks/blob/30e9fd788a4acb585566a008f4d5fff65ee06ce3/src/cluster/slot_import.h#L53-L59
--
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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1182151137
##########
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:
just avoid relinking all depended targets on .so. It's not necessary now, so I delete it.
--
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
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1414: support for cluster slot batch
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1181824040
##########
src/cluster/cluster.cc:
##########
@@ -641,6 +649,54 @@ 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::parseSlotRange(const std::string &slots_str, std::vector<std::pair<int, int>> &slots) {
+ if (slots_str.empty()) {
+ return {Status::NotOK, "Don't use empty slots."};
Review Comment:
Here you can simply say "No slots to parse" or something like that.
##########
src/cluster/cluster.cc:
##########
@@ -128,30 +123,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 = parseSlotRange(slots_str, slots);
Review Comment:
Is there always just one range of slots, according to the name of the function?
##########
src/cluster/cluster.cc:
##########
@@ -93,32 +93,27 @@ 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
Review Comment:
Typo in the phrase "By the way". You can omit it entirely because it doesn't add value to the sentence or explanation.
##########
src/cluster/cluster.cc:
##########
@@ -641,6 +649,54 @@ 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::parseSlotRange(const std::string &slots_str, std::vector<std::pair<int, int>> &slots) {
+ if (slots_str.empty()) {
+ return {Status::NotOK, "Don't use empty slots."};
+ }
+ 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 is_number = [&](const std::string &s) {
+#if __cplusplus >= 202002L
+ return std::ranges::all_of(s.begin(), s.end(), [](char c) { return isdigit(c) != 0; });
+#else
+ for (char c : s) {
+ if (isdigit(c) == 0) {
+ return false;
+ }
+ }
+ return true;
+#endif
+ };
+
+ // Parse all slots(include slot ranges)
+ for (const auto &slot_range : slot_ranges) {
+ if (is_number(slot_range)) {
Review Comment:
Personally, here I would split the input with `-` and then decide if it's a range or just a single slot number by checking the result.
Because right now you are checking all the characters, then splitting the string by `-` and checking all the characters of each substring.
##########
src/cluster/cluster.cc:
##########
@@ -641,6 +649,54 @@ 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::parseSlotRange(const std::string &slots_str, std::vector<std::pair<int, int>> &slots) {
+ if (slots_str.empty()) {
+ return {Status::NotOK, "Don't use empty slots."};
+ }
+ 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 is_number = [&](const std::string &s) {
+#if __cplusplus >= 202002L
+ return std::ranges::all_of(s.begin(), s.end(), [](char c) { return isdigit(c) != 0; });
+#else
+ for (char c : s) {
+ if (isdigit(c) == 0) {
+ return false;
+ }
+ }
+ return true;
+#endif
+ };
+
+ // Parse all slots(include slot ranges)
+ for (const auto &slot_range : slot_ranges) {
+ if (is_number(slot_range)) {
+ int s_start = stoi(slot_range);
+ assert(IsValidSlot(s_start));
+ slots.emplace_back(std::make_pair(s_start, s_start));
+ continue;
+ }
+
+ // parse slot range: "int1-int2" (satisfy: int1 <= int2 )
+ assert(slot_range.back() != '-');
+ std::vector<std::string> fields = util::Split(slot_range, "-");
+ assert(fields.size() == 2);
Review Comment:
Correct me if I'm wrong. `assert` works only in DEBUG build. Thus, in release build, if there is no `-` inside the string, the next line will try to access `fields[`]` and the service will crash.
In my opinion, such cases should be handled explicitly and return Status::NotOK instead of excessively using `assert`.
However, I would like to hear @PragmaTwice on that as an expert.
##########
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:
Maybe, it would be better to parse slot ranges inside `CommandClusterX::Parse` and then pass already parsed (and valid) numbers to the `SetSlot` function.
##########
src/cluster/cluster.cc:
##########
@@ -641,6 +649,54 @@ 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::parseSlotRange(const std::string &slots_str, std::vector<std::pair<int, int>> &slots) {
+ if (slots_str.empty()) {
+ return {Status::NotOK, "Don't use empty slots."};
+ }
+ 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 is_number = [&](const std::string &s) {
+#if __cplusplus >= 202002L
+ return std::ranges::all_of(s.begin(), s.end(), [](char c) { return isdigit(c) != 0; });
+#else
+ for (char c : s) {
+ if (isdigit(c) == 0) {
+ return false;
+ }
+ }
+ return true;
+#endif
+ };
+
+ // Parse all slots(include slot ranges)
+ for (const auto &slot_range : slot_ranges) {
+ if (is_number(slot_range)) {
+ int s_start = stoi(slot_range);
+ assert(IsValidSlot(s_start));
+ slots.emplace_back(std::make_pair(s_start, s_start));
+ continue;
+ }
+
+ // parse slot range: "int1-int2" (satisfy: int1 <= int2 )
+ assert(slot_range.back() != '-');
+ std::vector<std::string> fields = util::Split(slot_range, "-");
+ assert(fields.size() == 2);
+ if (is_number(fields[0]) && is_number(fields[1])) {
+ int s_start = stoi(fields[0]), s_end = stoi(fields[1]);
+ assert((s_start <= s_end) && IsValidSlot(s_end));
+ slots.emplace_back(std::make_pair(s_start, s_end));
Review Comment:
Did you forget to insert `continue` after `emplace_back`?
##########
src/cluster/cluster.cc:
##########
@@ -641,6 +649,54 @@ 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::parseSlotRange(const std::string &slots_str, std::vector<std::pair<int, int>> &slots) {
+ if (slots_str.empty()) {
+ return {Status::NotOK, "Don't use empty slots."};
+ }
+ 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 is_number = [&](const std::string &s) {
+#if __cplusplus >= 202002L
+ return std::ranges::all_of(s.begin(), s.end(), [](char c) { return isdigit(c) != 0; });
+#else
+ for (char c : s) {
+ if (isdigit(c) == 0) {
+ return false;
+ }
+ }
+ return true;
+#endif
+ };
+
+ // Parse all slots(include slot ranges)
+ for (const auto &slot_range : slot_ranges) {
+ if (is_number(slot_range)) {
+ int s_start = stoi(slot_range);
Review Comment:
Here you can use the `ParseInt` function from `parse_util.h`.
--
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
[GitHub] [incubator-kvrocks] infdahai commented on pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#issuecomment-1531021738
> Thanks for your contribution!
>
> You can run `./x.py format` to format the code and let the CI proceed.
If I exec `./x.py format`, I get a irrelevant result.(Not important though, just ignored).
--
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
[GitHub] [incubator-kvrocks] caipengbo commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1185656370
##########
src/cluster/cluster.cc:
##########
@@ -93,32 +83,27 @@ 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::SetSlotRanges(const std::vector<std::pair<int, int>> &slot_ranges, const std::string &node_id,
Review Comment:
Personally, I think it's better to use alias declaration(Like `using SlotRange = std::pair<int, int>` ) or struct, because I see `std::pair<int, int>` appear 6 times.
--
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
[GitHub] [incubator-kvrocks] caipengbo commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1185736913
##########
src/cluster/cluster.cc:
##########
@@ -135,23 +120,29 @@ Status Cluster::SetSlot(int slot, const std::string &node_id, int64_t new_versio
// 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 (auto [s_start, s_end] : slot_ranges) {
+ for (int slot = s_start; slot <= s_end; slot++) {
+ 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);
Review Comment:
There is this problem, and we can persist it.
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1182201209
##########
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:
Thanks. Got your point.
--
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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1182326391
##########
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:
OK, I will remove this part.
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#issuecomment-1531051634
CI seems still blocked in format. Not sure your clang-format version, but you can try clang-format 12, which is used in CI.
https://github.com/llvm/llvm-project/releases/tag/llvmorg-12.0.1
And I can confirm that clang-format 14 on my local has also no issue.
--
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
[GitHub] [incubator-kvrocks] mapleFU commented on a diff in pull request #1414: support for cluster slot batch
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1181831949
##########
src/cluster/cluster.cc:
##########
@@ -641,6 +649,54 @@ 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::parseSlotRange(const std::string &slots_str, std::vector<std::pair<int, int>> &slots) {
+ if (slots_str.empty()) {
+ return {Status::NotOK, "Don't use empty slots."};
+ }
+ 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 is_number = [&](const std::string &s) {
Review Comment:
Why not use `src/common/parse_util.h`?
##########
src/cluster/cluster.cc:
##########
@@ -641,6 +649,54 @@ 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::parseSlotRange(const std::string &slots_str, std::vector<std::pair<int, int>> &slots) {
+ if (slots_str.empty()) {
+ return {Status::NotOK, "Don't use empty slots."};
+ }
+ 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 is_number = [&](const std::string &s) {
+#if __cplusplus >= 202002L
+ return std::ranges::all_of(s.begin(), s.end(), [](char c) { return isdigit(c) != 0; });
+#else
+ for (char c : s) {
+ if (isdigit(c) == 0) {
+ return false;
+ }
+ }
+ return true;
+#endif
+ };
+
+ // Parse all slots(include slot ranges)
+ for (const auto &slot_range : slot_ranges) {
+ if (is_number(slot_range)) {
+ int s_start = stoi(slot_range);
+ assert(IsValidSlot(s_start));
+ slots.emplace_back(std::make_pair(s_start, s_start));
+ continue;
+ }
+
+ // parse slot range: "int1-int2" (satisfy: int1 <= int2 )
+ assert(slot_range.back() != '-');
+ std::vector<std::string> fields = util::Split(slot_range, "-");
+ assert(fields.size() == 2);
Review Comment:
ditto
##########
src/cluster/cluster.cc:
##########
@@ -641,6 +649,54 @@ 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::parseSlotRange(const std::string &slots_str, std::vector<std::pair<int, int>> &slots) {
+ if (slots_str.empty()) {
+ return {Status::NotOK, "Don't use empty slots."};
+ }
+ 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 is_number = [&](const std::string &s) {
+#if __cplusplus >= 202002L
+ return std::ranges::all_of(s.begin(), s.end(), [](char c) { return isdigit(c) != 0; });
+#else
+ for (char c : s) {
+ if (isdigit(c) == 0) {
+ return false;
+ }
+ }
+ return true;
+#endif
+ };
+
+ // Parse all slots(include slot ranges)
+ for (const auto &slot_range : slot_ranges) {
+ if (is_number(slot_range)) {
+ int s_start = stoi(slot_range);
+ assert(IsValidSlot(s_start));
+ slots.emplace_back(std::make_pair(s_start, s_start));
+ continue;
+ }
+
+ // parse slot range: "int1-int2" (satisfy: int1 <= int2 )
+ assert(slot_range.back() != '-');
Review Comment:
If user pass a ambigious string, would this causing fatal?
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "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
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#issuecomment-1531026490
> If I exec `./x.py format`, I get a irrelevant result.(Not important though, just ignored). 
Could you
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#issuecomment-1531027477
Sorry for the wrong close-button click. :rofl:
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1185743984
##########
src/cluster/cluster_defs.h:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include "cluster/redis_slot.h"
+
+enum {
+ kClusterMaster = 1,
+ kClusterSlave = 2,
+ kClusterNodeIdLen = 40,
+ kClusterPortIncr = 10000,
+ kClusterSlots = HASH_SLOTS_SIZE,
+};
+
+inline constexpr const char *errInvalidNodeID = "Invalid cluster node id";
+inline constexpr const char *errInvalidSlotID = "Invalid slot id";
+inline constexpr const char *errSlotOutOfRange = "Slot is out of range";
+inline constexpr const char *errInvalidClusterVersion = "Invalid cluster version";
+inline constexpr const char *errSlotOverlapped = "Slot distribution is overlapped";
+inline constexpr const char *errNoMasterNode = "The node isn't a master";
+inline constexpr const char *errClusterNoInitialized = "CLUSTERDOWN The cluster is not initialized";
+inline constexpr const char *errInvalidClusterNodeInfo = "Invalid cluster nodes info";
+inline constexpr const char *errInvalidImportState = "Invalid import state";
+
+using SlotRange = std::pair<int, int>;
Review Comment:
I add "\n" to the line in local env but It doesn't seem to see it in github. If I add two `\n` with increasing two line nums and then I get the above situation, but `clang-format` will fix this.
https://github.com/apache/incubator-kvrocks/blob/30e9fd788a4acb585566a008f4d5fff65ee06ce3/src/cluster/slot_import.h#L53-L59
--
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
[GitHub] [incubator-kvrocks] caipengbo commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1185741503
##########
src/cluster/cluster.cc:
##########
@@ -135,23 +120,29 @@ Status Cluster::SetSlot(int slot, const std::string &node_id, int64_t new_versio
// 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 (auto [s_start, s_end] : slot_ranges) {
+ for (int slot = s_start; slot <= s_end; slot++) {
+ 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);
Review Comment:
After we merge this PR, you can ask an issue and I will solve this problem, which I have solved in production environment.
--
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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1185734066
##########
src/cluster/cluster_defs.h:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include "cluster/redis_slot.h"
+
+enum {
Review Comment:
I move the enum struct and strings defined in `cluster` class.
It reduces dependencies and we can add this header in another module, such as `commander.h`.
Perhaps we can move many strings like this if we want to use these in other classs.
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1185733754
##########
src/cluster/cluster_defs.h:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include "cluster/redis_slot.h"
+
+enum {
+ kClusterMaster = 1,
+ kClusterSlave = 2,
+ kClusterNodeIdLen = 40,
+ kClusterPortIncr = 10000,
+ kClusterSlots = HASH_SLOTS_SIZE,
+};
+
+inline constexpr const char *errInvalidNodeID = "Invalid cluster node id";
+inline constexpr const char *errInvalidSlotID = "Invalid slot id";
+inline constexpr const char *errSlotOutOfRange = "Slot is out of range";
+inline constexpr const char *errInvalidClusterVersion = "Invalid cluster version";
+inline constexpr const char *errSlotOverlapped = "Slot distribution is overlapped";
+inline constexpr const char *errNoMasterNode = "The node isn't a master";
+inline constexpr const char *errClusterNoInitialized = "CLUSTERDOWN The cluster is not initialized";
+inline constexpr const char *errInvalidClusterNodeInfo = "Invalid cluster nodes info";
+inline constexpr const char *errInvalidImportState = "Invalid import state";
+
+using SlotRange = std::pair<int, int>;
Review Comment:
```suggestion
using SlotRange = std::pair<int, int>;
```
better to have a newline in the end of the file.
--
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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1185731096
##########
src/cluster/cluster.cc:
##########
@@ -93,32 +83,27 @@ 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::SetSlotRanges(const std::vector<std::pair<int, int>> &slot_ranges, const std::string &node_id,
Review Comment:
OK, I use SlotRange instead of std::pair<int,int>.
And it lefts the `start` and `end` integers in SlotInfo struct. Maybe we think about it later.
```cpp
// TODO: replace integer `start`, `end` with SlotRange
struct SlotInfo {
int start;
int end;
struct NodeInfo {
std::string host;
int port;
std::string id;
};
std::vector<NodeInfo> nodes;
};
```
--
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
[GitHub] [incubator-kvrocks] caipengbo commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1185736913
##########
src/cluster/cluster.cc:
##########
@@ -135,23 +120,29 @@ Status Cluster::SetSlot(int slot, const std::string &node_id, int64_t new_versio
// 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 (auto [s_start, s_end] : slot_ranges) {
+ for (int slot = s_start; slot <= s_end; slot++) {
+ 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);
Review Comment:
There is indeed this problem and we can persist it to Propagate CF.
--
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
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1185743984
##########
src/cluster/cluster_defs.h:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include "cluster/redis_slot.h"
+
+enum {
+ kClusterMaster = 1,
+ kClusterSlave = 2,
+ kClusterNodeIdLen = 40,
+ kClusterPortIncr = 10000,
+ kClusterSlots = HASH_SLOTS_SIZE,
+};
+
+inline constexpr const char *errInvalidNodeID = "Invalid cluster node id";
+inline constexpr const char *errInvalidSlotID = "Invalid slot id";
+inline constexpr const char *errSlotOutOfRange = "Slot is out of range";
+inline constexpr const char *errInvalidClusterVersion = "Invalid cluster version";
+inline constexpr const char *errSlotOverlapped = "Slot distribution is overlapped";
+inline constexpr const char *errNoMasterNode = "The node isn't a master";
+inline constexpr const char *errClusterNoInitialized = "CLUSTERDOWN The cluster is not initialized";
+inline constexpr const char *errInvalidClusterNodeInfo = "Invalid cluster nodes info";
+inline constexpr const char *errInvalidImportState = "Invalid import state";
+
+using SlotRange = std::pair<int, int>;
Review Comment:
I add "\n" to the line in local env but It doesn't seem to see it in github
--
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
[GitHub] [incubator-kvrocks] infdahai commented on pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#issuecomment-1535830806
> LGTM, I think we can remove TODO comments, then submit an issue if you feel it's necessary.
OK
--
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
[GitHub] [incubator-kvrocks] infdahai commented on pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#issuecomment-1531035941
> > If I exec `./x.py format`, I get a irrelevant result.(Not important though, just ignored). 
>
> Could you provide the version of clang-format you are using?
>
> Anyway, I think you can firstly manually ignore this unrelated change as a workaround.
yes, I will propose a new pr for this `clang-tidy`.
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1414: support slot batch for CLUSTERX SETSLOT
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1414:
URL: https://github.com/apache/incubator-kvrocks/pull/1414#discussion_r1182207772
##########
src/cluster/redis_slot.h:
##########
@@ -19,6 +19,7 @@
*/
#pragma once
+#include <cstdint>
Review Comment:
Got it. Thanks for your demo.
--
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