[GitHub] [incubator-kvrocks] mapleFU commented on a diff in pull request #1219: Persist the cluster nodes info after applying the cluster topology

[GitBox <gi...@apache.org>]

mapleFU commented on code in PR #1219:
URL: https://github.com/apache/incubator-kvrocks/pull/1219#discussion_r1064519857


##########
src/server/server.cc:
##########
@@ -137,12 +137,17 @@ Status Server::Start() {
   }
 
   if (config_->cluster_enabled) {
+    auto s = cluster_->LoadClusterNodes(config_->NodesFilePath());

Review Comment:
   Hi hulk, the reload make kvrocks a bit more trickey, if:
   1. a server(A) mount on a disk crashed
   2. user add some machines, and update the cluster version
   3. A restarted, load the file, but has lower version. Previously, it can recover and load from other machines, but now, user need drop all it's data.
   
   How can we handle this problem?



##########
src/cluster/cluster.cc:
##########
@@ -511,37 +552,118 @@ std::string Cluster::GenNodesDescription() {
     }
   }
 
-  std::string nodes_desc;
   for (const auto &item : nodes_) {
     const std::shared_ptr<ClusterNode> n = item.second;
+    if (n->slots_info_.size() > 0) n->slots_info_.pop_back();  // Remove last space
+  }
+}
+
+std::string Cluster::GenNodesInfo() {
+  UpdateSlotsInfo();
 
+  std::string nodes_info;
+  for (const auto &item : nodes_) {
+    const std::shared_ptr<ClusterNode> n = item.second;
     std::string node_str;
-    // ID, host, port
+    // ID
     node_str.append(n->id_ + " ");
-    node_str.append(fmt::format("{}:{}@{} ", n->host_, n->port_, n->port_ + kClusterPortIncr));
+    // Host + Port
+    node_str.append(fmt::format("{} {} ", n->host_, n->port_));
 
-    // Flags
-    if (n->id_ == myid_) node_str.append("myself,");
+    // Role
     if (n->role_ == kClusterMaster) {
       node_str.append("master - ");
     } else {
       node_str.append("slave " + n->master_id_ + " ");
     }
 
-    // Ping sent, pong received, config epoch, link status
-    auto now = Util::GetTimeStampMS();
-    node_str.append(fmt::format("{} {} {} connected", now - 1, now, version_));
-
     // Slots
-    if (n->slots_info_.size() > 0) n->slots_info_.pop_back();  // Trim space
     if (n->role_ == kClusterMaster && n->slots_info_.size() > 0) {
       node_str.append(" " + n->slots_info_);
     }
-    n->slots_info_.clear();  // Reset
+    nodes_info.append(node_str + "\n");
+  }
+  return nodes_info;
+}
 
-    nodes_desc.append(node_str + "\n");
+Status Cluster::DumpClusterNodes(const std::string &file) {
+  // Parse and validate the cluster nodes string before dumping into file
+  std::string tmp_path = file + ".tmp";
+  remove(tmp_path.data());
+  std::ofstream output_file(tmp_path, std::ios::out);
+  output_file << "# version\n";
+  output_file << fmt::format("{}\n", version_);
+  output_file << "# id\n";
+  output_file << fmt::format("{}\n", myid_);
+  output_file << "# nodes\n";
+  output_file << GenNodesInfo();
+  output_file.close();
+  if (rename(tmp_path.data(), file.data()) < 0) {
+    return {Status::NotOK, fmt::format("rename file encounter error: {}", strerror(errno))};
   }
-  return nodes_desc;
+  return Status::OK();
+}
+
+LoadState parseLoadState(const std::string &in) {
+  if (in == "version") {
+    return LoadState::Version;
+  } else if (in == "id") {
+    return LoadState::ID;
+  } else if (in == "nodes") {
+    return LoadState::NodesInfo;
+  } else {
+    return LoadState::Unknown;
+  }
+}
+
+Status Cluster::LoadClusterNodes(const std::string &file_path) {
+  if (rocksdb::Env::Default()->FileExists(file_path).IsNotFound()) {
+    return Status::OK();

Review Comment:
   Hi, when first start the cluster, we would meet that message, so it may confuse the user?



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