You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "torwig (via GitHub)" <gi...@apache.org> on 2023/05/01 19:44:22 UTC
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1414: support for cluster slot batch
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