You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "RiversJin (via GitHub)" <gi...@apache.org> on 2025/03/23 11:50:11 UTC
[PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
RiversJin opened a new pull request, #2850:
URL: https://github.com/apache/kvrocks/pull/2850
it solves https://github.com/apache/kvrocks/issues/2841
--
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
Re: [PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
Posted by "RiversJin (via GitHub)" <gi...@apache.org>.
RiversJin commented on PR #2850:
URL: https://github.com/apache/kvrocks/pull/2850#issuecomment-2760517209
> I found that this PR introduces many code styles and fixes, except for the disconnecting feature. Would you mind separating them into a few dedicated PRs that would be easy to review and clarify the context?
@git-hulk Sure! The unrelated tweaks were legacy from our internal codebase. I've removed the obvious ones, but the iterator-based index optimization in Cluster::nodes_ (avoiding double lookups) is interleaved in some places. It's technically out of scope, but actually improves performance. but need some time to remove those...
--
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
Re: [PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #2850:
URL: https://github.com/apache/kvrocks/pull/2850#issuecomment-2746185509
Thank you for your contribution! Could you add some test cases for 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
Re: [PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2850:
URL: https://github.com/apache/kvrocks/pull/2850#discussion_r2009647660
##########
src/cluster/cluster.cc:
##########
@@ -197,8 +214,11 @@ Status Cluster::SetClusterNodes(const std::string &nodes_str, int64_t version, b
}
myself_ = nullptr;
- if (!myid_.empty() && nodes_.find(myid_) != nodes_.end()) {
- myself_ = nodes_[myid_];
+ if (!myid_.empty()) {
+ auto it = nodes_->find(myid_);
+ if (it != nodes_->end()) {
Review Comment:
`if(auto it = ;`
##########
src/cluster/cluster.cc:
##########
@@ -96,11 +105,13 @@ Status Cluster::SetSlotRanges(const std::vector<SlotRange> &slot_ranges, const s
}
// Get the node which we want to assign slots into it
- std::shared_ptr<ClusterNode> to_assign_node = nodes_[node_id];
- if (to_assign_node == nullptr) {
+ auto it = nodes_->find(node_id);
+ if (it == nodes_->end()) {
return {Status::NotOK, "No this node in the cluster"};
}
+ auto to_assign_node = it->second;
Review Comment:
```suggestion
std::shared_ptr<ClusterNode> to_assign_node;
if (auto it = nodes_.find(); it != nodes_.end()) {
to_assign_node = ..
} else {
return {Status::NotOK, "No this node in the cluster"};
}
```
##########
src/cluster/cluster.cc:
##########
@@ -23,24 +23,33 @@
#include <config/config_util.h>
#include <array>
+#include <cstdint>
#include <cstring>
#include <fstream>
#include <memory>
+#include <string>
+#include <string_view>
#include <vector>
#include "cluster/cluster_defs.h"
#include "commands/commander.h"
#include "common/io_util.h"
+#include "fmt/base.h"
#include "fmt/format.h"
#include "parse_util.h"
#include "replication.h"
#include "server/server.h"
#include "string_util.h"
#include "time_util.h"
-ClusterNode::ClusterNode(std::string id, std::string host, int port, int role, std::string master_id,
- const std::bitset<kClusterSlots> &slots)
- : id(std::move(id)), host(std::move(host)), port(port), role(role), master_id(std::move(master_id)), slots(slots) {}
+ClusterNode::ClusterNode(std::string &&id, std::string &&host, int port, int role, std::string &&master_id,
Review Comment:
why these are changed?
##########
src/cluster/cluster.cc:
##########
@@ -252,11 +272,14 @@ Status Cluster::SetMasterSlaveRepl() {
srv_->slot_migrator->SetStopMigrationFlag(false);
LOG(INFO) << "Change server role to master, restart migration task";
}
+ if (!is_slave) {
+ srv_->CleanupOrphanSlaves(version_, *nodes_);
+ }
Review Comment:
So this is what this patch actually does?
##########
src/cluster/cluster.cc:
##########
@@ -68,8 +77,8 @@ Status Cluster::SetNodeId(const std::string &node_id) {
myid_ = node_id;
// Already has cluster topology
- if (version_ >= 0 && nodes_.find(node_id) != nodes_.end()) {
- myself_ = nodes_[myid_];
+ if (version_ >= 0 && nodes_->count(node_id) > 0) {
+ myself_ = nodes_->at(node_id);
Review Comment:
if the performance is important, why not use `find()` and iterator based operations? Which could make it just search once
##########
src/server/redis_connection.h:
##########
@@ -134,12 +139,18 @@ class Connection : public EvbufCallbackBase<Connection> {
void SetLastCmd(std::string cmd) { last_cmd_ = std::move(cmd); }
std::string GetIP() const { return ip_; }
uint32_t GetPort() const { return port_; }
- void SetListeningPort(int port) { listening_port_ = port; }
- int GetListeningPort() const { return listening_port_; }
- void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
- std::string GetAnnounceIP() const { return !announce_ip_.empty() ? announce_ip_ : ip_; }
- uint32_t GetAnnouncePort() const { return listening_port_ != 0 ? listening_port_ : port_; }
- std::string GetAnnounceAddr() const { return GetAnnounceIP() + ":" + std::to_string(GetAnnouncePort()); }
+
+ void SetPeerInfo(std::unique_ptr<PeerInfo> &&peer_info) { peer_info_ = std::move(peer_info); }
+
+ const PeerInfo *GetPeerInfo() {
+ if (peer_info_) {
+ return peer_info_.get();
+ }
+
+ SetPeerInfo(std::make_unique<PeerInfo>(ip_, port_, "", -1));
+ return peer_info_.get();
Review Comment:
Emmm when would this happens?
##########
src/server/redis_connection.h:
##########
@@ -217,4 +229,44 @@ class Connection : public EvbufCallbackBase<Connection> {
RESP protocol_version_ = RESP::v2;
};
+class PeerInfo {
+ public:
+ PeerInfo() = default;
+ ~PeerInfo() = default;
+
+ PeerInfo(std::string_view ip, uint32_t port, std::string_view peer_id, int64_t peer_version)
+ : port_(port), peer_version_(peer_version) {
+ if (peer_id.empty()) {
+ str_ = fmt::format("{}:{}", ip, port);
+ } else {
+ str_ = fmt::format("{}:{} ({}@{})", ip, port, peer_id, peer_version);
+ }
+
+ addr_ = std::string_view(str_).substr(0, str_.find(' '));
+ ip_ = std::string_view(str_).substr(0, ip.length());
+
+ if (!peer_id.empty()) {
+ peer_id_ = std::string_view(str_).substr(addr_.length() + 1, peer_id.length());
+ }
+ }
+
+ std::string_view GetIP() const { return ip_; }
+ uint32_t GetPort() const { return port_; }
+
+ std::string_view GetStringView() const { return str_; }
+ std::string_view GetPeerID() const { return peer_id_; }
+ std::string_view GetAddr() const { return addr_; }
+ int64_t GetPeerVersion() const { return peer_version_; }
+
+ private:
+ std::string_view ip_;
+ std::string_view addr_;
Review Comment:
This looks really unsafe..Who would owns memory for them?
##########
src/server/server.cc:
##########
@@ -354,6 +355,27 @@ void Server::DisconnectSlaves() {
}
}
+void Server::CleanupOrphanSlaves(int64_t version, const ClusterNodes &nodes) {
+ std::lock_guard<std::mutex> lg(slave_threads_mu_);
+
+ for (auto &slave_thread : slave_threads_) {
+ const auto peer_info = slave_thread->GetConn()->GetPeerInfo();
+ auto peer_version = peer_info->GetPeerVersion();
+ if (peer_version < 0 || peer_version > version) {
+ // The peer version is greater than the current version,
Review Comment:
what does `peer_version < 0` means?
##########
src/server/server.cc:
##########
@@ -354,6 +355,27 @@ void Server::DisconnectSlaves() {
}
}
+void Server::CleanupOrphanSlaves(int64_t version, const ClusterNodes &nodes) {
+ std::lock_guard<std::mutex> lg(slave_threads_mu_);
+
+ for (auto &slave_thread : slave_threads_) {
+ const auto peer_info = slave_thread->GetConn()->GetPeerInfo();
Review Comment:
do we need check `GetPeerInfo != nullptr`?
--
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
Re: [PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
Posted by "RiversJin (via GitHub)" <gi...@apache.org>.
RiversJin commented on code in PR #2850:
URL: https://github.com/apache/kvrocks/pull/2850#discussion_r2009093589
##########
src/commands/cmd_replication.cc:
##########
@@ -166,41 +177,66 @@ class CommandReplConf : public Commander {
return Commander::Parse(args);
}
- Status ParseParam(const std::string &option, const std::string &value) {
+ Status ParseParam(std::string_view option, std::string_view value) {
if (option == "listening-port") {
auto parse_result = ParseInt<int>(value, NumericRange<int>{1, PORT_LIMIT - 1}, 10);
if (!parse_result) {
return {Status::RedisParseErr, "listening-port should be number or out of range"};
}
-
port_ = *parse_result;
- } else if (option == "ip-address") {
- if (value == "") {
+ return Status::OK();
+ }
+
+ if (option == "ip-address") {
+ if (value.empty()) {
return {Status::RedisParseErr, "ip-address should not be empty"};
}
- ip_address_ = value;
- } else {
- return {Status::RedisParseErr, errUnknownOption};
+ ip_ = value;
+ return Status::OK();
}
- return Status::OK();
+ if (option == "peer-id") {
+ if (value.empty()) {
+ return {Status::RedisParseErr, "peer-id should not be empty"};
+ }
+ peer_id_ = value;
+ return Status::OK();
+ }
+
+ if (option == "version") {
+ auto parse_result = ParseInt<int64_t>(value, 10);
+ if (!parse_result) {
+ return {Status::RedisParseErr, "version should be number"};
+ }
+ peer_version_ = *parse_result;
+ return Status::OK();
+ }
+
+ return {Status::RedisParseErr, errUnknownOption};
}
Status Execute([[maybe_unused]] engine::Context &ctx, [[maybe_unused]] Server *srv, Connection *conn,
std::string *output) override {
- if (port_ != 0) {
Review Comment:
Based on the `replconf` command definition, it's impossible to reach this code path without `port_` being set, so I've removed the validation check for `port_`.
--
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
Re: [PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #2850:
URL: https://github.com/apache/kvrocks/pull/2850#issuecomment-2763381324
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2850) **Quality Gate passed**
Issues
 [6 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2850&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2850&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2850&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [51.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2850&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2850&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2850)
--
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
Re: [PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #2850:
URL: https://github.com/apache/kvrocks/pull/2850#issuecomment-2768834059
Yeah these changes in `parse_utils.h` looks good to me, so maybe you can open a separate PR for 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
Re: [PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2850:
URL: https://github.com/apache/kvrocks/pull/2850#discussion_r2018070339
##########
src/cluster/cluster.cc:
##########
@@ -977,3 +991,13 @@ Status Cluster::Reset() {
unlink(srv_->GetConfig()->NodesFilePath().data());
return Status::OK();
}
+
+// Note that if current version is lower than the given version,
+// it can't be determined whether the node is in the cluster, so just regard it as in the cluster.
+bool Cluster::IsInCluster(const std::string &node_id, int64_t version) const {
+ if (version < 0 || this->version_ < version) {
+ return true;
+ }
+
+ return nodes_->count(node_id) > 0;
+}
Review Comment:
```suggestion
}
```
##########
src/cluster/cluster_defs.h:
##########
@@ -41,6 +41,7 @@ inline constexpr const char *errNoMasterNode = "The node isn't a master";
inline constexpr const char *errClusterNoInitialized = "The cluster is not initialized";
inline constexpr const char *errInvalidClusterNodeInfo = "Invalid cluster nodes info";
inline constexpr const char *errInvalidImportState = "Invalid import state";
+inline constexpr const char *errYouAreFired = "You are fired";
Review Comment:
I think it's not a right wording. Could be something like `disconnected due to the topology change` or others.
--
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
Re: [PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
Posted by "RiversJin (via GitHub)" <gi...@apache.org>.
RiversJin commented on PR #2850:
URL: https://github.com/apache/kvrocks/pull/2850#issuecomment-2747128034
> Thank you for your contribution! Could you add some test cases for it?
@PragmaTwice After a closer look, this PR doesn't introduce any new changes other than disconnecting data replication for nodes removed from the cluster. I added a small snippet of assertion code in cluster_test.go for this scenario. Do you think there are any other areas that might need more testing?
--
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
Re: [PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
Posted by "RiversJin (via GitHub)" <gi...@apache.org>.
RiversJin commented on PR #2850:
URL: https://github.com/apache/kvrocks/pull/2850#issuecomment-2746216951
> Thank you for your contribution! Could you add some test cases for it?
Of course, but save the boring tasks for weekdays :P.
--
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
Re: [PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2850:
URL: https://github.com/apache/kvrocks/pull/2850#discussion_r2015550616
##########
src/cluster/cluster.cc:
##########
@@ -23,24 +23,33 @@
#include <config/config_util.h>
#include <array>
+#include <cstdint>
#include <cstring>
#include <fstream>
#include <memory>
+#include <string>
+#include <string_view>
#include <vector>
#include "cluster/cluster_defs.h"
#include "commands/commander.h"
#include "common/io_util.h"
+#include "fmt/base.h"
#include "fmt/format.h"
#include "parse_util.h"
#include "replication.h"
#include "server/server.h"
#include "string_util.h"
#include "time_util.h"
-ClusterNode::ClusterNode(std::string id, std::string host, int port, int role, std::string master_id,
- const std::bitset<kClusterSlots> &slots)
- : id(std::move(id)), host(std::move(host)), port(port), role(role), master_id(std::move(master_id)), slots(slots) {}
+ClusterNode::ClusterNode(std::string &&id, std::string &&host, int port, int role, std::string &&master_id,
Review Comment:
Some notes:
1. for `std::bitset`, in most impl it is stored on the stack memory instead heap so move ctor has no difference from the copy ctor. usually you don't need to move it.
2. for `ClassA(std::string x) : x(x) {}`, you can pass either lval or rval to it. for rval, it involves two move ctors. And for `ClassA(std::string &&x) : x(x) {}`, you can only pass rval to it, and also the performance difference is quite little or none.
So these changes are not so useful.
--
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
Re: [PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2850:
URL: https://github.com/apache/kvrocks/pull/2850#discussion_r2015550616
##########
src/cluster/cluster.cc:
##########
@@ -23,24 +23,33 @@
#include <config/config_util.h>
#include <array>
+#include <cstdint>
#include <cstring>
#include <fstream>
#include <memory>
+#include <string>
+#include <string_view>
#include <vector>
#include "cluster/cluster_defs.h"
#include "commands/commander.h"
#include "common/io_util.h"
+#include "fmt/base.h"
#include "fmt/format.h"
#include "parse_util.h"
#include "replication.h"
#include "server/server.h"
#include "string_util.h"
#include "time_util.h"
-ClusterNode::ClusterNode(std::string id, std::string host, int port, int role, std::string master_id,
- const std::bitset<kClusterSlots> &slots)
- : id(std::move(id)), host(std::move(host)), port(port), role(role), master_id(std::move(master_id)), slots(slots) {}
+ClusterNode::ClusterNode(std::string &&id, std::string &&host, int port, int role, std::string &&master_id,
Review Comment:
Some notes:
1. for `std::bitset`, in most impl it is stored on the stack memory instead heap so move ctor has no difference from the copy ctor. usually you don't need to move it.
2. for `ClassA(std::string x) : x(x) {}`, you can pass either lval or rval to it. for rval, it involves two move ctor. And for `ClassA(std::string &&x) : x(x) {}`, you can only pass rval to it, and also the performance difference is quite little or none.
So these changes are not so useful.
--
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
Re: [PR] feat(cluster): disconnect replication connections to orphan slaves [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #2850:
URL: https://github.com/apache/kvrocks/pull/2850#issuecomment-2760465796
@RiversJin, I found that this PR introduces many code styles and fixes, except for the disconnecting feature. Would you mind separating them into a few dedicated PRs that would be easy to review and clarify the context?
--
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