You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "paragor (via GitHub)" <gi...@apache.org> on 2023/03/09 06:43:47 UTC
[GitHub] [incubator-kvrocks] paragor opened a new pull request, #1303: Support replica-announce-ip and replica-announce-port
paragor opened a new pull request, #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303
Add support
- replica-announce-ip
- replica-announce-port
Generally, it is useful for NAT.
And for me - it is useful for deploy with redis sentinel in k8s environment (when kvrocks is statefulset), because in rollout restart pod's ip changes. And replica-announce-ip can expose dns name that will be the same between restarts.
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130622998
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,9 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
Review Comment:
You can help to add this logic if you don't mind.
--
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] paragor commented on a diff in pull request #1303: Support replica-announce-ip and replica-announce-port
Posted by "paragor (via GitHub)" <gi...@apache.org>.
paragor commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130826067
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,11 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
+ std::string GetAnnounceIPOrIP() { return !announce_ip_.empty() ? announce_ip_ : ip_; }
+ std::string GetAnnounceAddr() {
+ return GetAnnounceIPOrIP() + ":" + std::to_string(listening_port_ > 0 ? listening_port_ : port_);
Review Comment:
i was thinking about old versions of kvrocks that can't send listening_port
but now i see git blame and it seems listening_port_ will always > 0. So i just will use to_string(listening_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
[GitHub] [incubator-kvrocks] paragor commented on a diff in pull request #1303: Support replica-announce-ip and replica-announce-port
Posted by "paragor (via GitHub)" <gi...@apache.org>.
paragor commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130843583
##########
src/cluster/replication.cc:
##########
@@ -425,18 +425,35 @@ ReplicationThread::CBState ReplicationThread::checkDBNameReadCB(bufferevent *bev
ReplicationThread::CBState ReplicationThread::replConfWriteCB(bufferevent *bev, void *ctx) {
auto self = static_cast<ReplicationThread *>(ctx);
- send_string(bev,
- Redis::MultiBulkString({"replconf", "listening-port", std::to_string(self->srv_->GetConfig()->port)}));
+ auto config = self->srv_->GetConfig();
+
+ auto port = config->replica_announce_port > 0 ? config->replica_announce_port : config->port;
+ if (!self->next_try_without_announce_ip_address_ && !config->replica_announce_ip.empty()) {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port), "ip-address",
+ config->replica_announce_ip}));
+ } else {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port)}));
+ }
+
self->repl_state_.store(kReplReplConf, std::memory_order_relaxed);
LOG(INFO) << "[replication] replconf request was sent, waiting for response";
return CBState::NEXT;
}
ReplicationThread::CBState ReplicationThread::replConfReadCB(bufferevent *bev, void *ctx) {
+ auto self = static_cast<ReplicationThread *>(ctx);
auto input = bufferevent_get_input(bev);
UniqueEvbufReadln line(input, EVBUFFER_EOL_CRLF_STRICT);
if (!line) return CBState::AGAIN;
+ // on unknown option: first try without announce ip, if it fails again - do nothing (to prevent infinite loop)
+ if (line[0] == '-' && isUnknownOption(line.get()) && !self->next_try_without_announce_ip_address_) {
Review Comment:
done
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,11 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
+ std::string GetAnnounceIPOrIP() { return !announce_ip_.empty() ? announce_ip_ : ip_; }
+ std::string GetAnnounceAddr() {
+ return GetAnnounceIPOrIP() + ":" + std::to_string(listening_port_ > 0 ? listening_port_ : port_);
Review Comment:
done
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,11 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
+ std::string GetAnnounceIPOrIP() { return !announce_ip_.empty() ? announce_ip_ : ip_; }
Review Comment:
done
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130620401
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,9 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
+ std::string GetAnnounceIP() { return announce_ip_; }
+ std::string GetAnnounceIPOrIP() { return !announce_ip_.empty() ? announce_ip_ : ip_; }
Review Comment:
Yes, it's using indeed now.
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130607885
##########
src/cluster/replication.cc:
##########
@@ -425,18 +425,35 @@ ReplicationThread::CBState ReplicationThread::checkDBNameReadCB(bufferevent *bev
ReplicationThread::CBState ReplicationThread::replConfWriteCB(bufferevent *bev, void *ctx) {
auto self = static_cast<ReplicationThread *>(ctx);
- send_string(bev,
- Redis::MultiBulkString({"replconf", "listening-port", std::to_string(self->srv_->GetConfig()->port)}));
+ auto config = self->srv_->GetConfig();
+
+ auto port = config->replica_announce_port > 0 ? config->replica_announce_port : config->port;
+ if (!self->next_try_without_announce_ip_address_ && !config->replica_announce_ip.empty()) {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port), "ip-address",
+ config->replica_announce_ip}));
+ } else {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port)}));
Review Comment:
I think this place is fine since we are using this initialized vector style in many other places.
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130636197
##########
src/cluster/replication.cc:
##########
@@ -425,18 +425,35 @@ ReplicationThread::CBState ReplicationThread::checkDBNameReadCB(bufferevent *bev
ReplicationThread::CBState ReplicationThread::replConfWriteCB(bufferevent *bev, void *ctx) {
auto self = static_cast<ReplicationThread *>(ctx);
- send_string(bev,
- Redis::MultiBulkString({"replconf", "listening-port", std::to_string(self->srv_->GetConfig()->port)}));
+ auto config = self->srv_->GetConfig();
+
+ auto port = config->replica_announce_port > 0 ? config->replica_announce_port : config->port;
+ if (!self->next_try_without_announce_ip_address_ && !config->replica_announce_ip.empty()) {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port), "ip-address",
+ config->replica_announce_ip}));
+ } else {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port)}));
+ }
+
self->repl_state_.store(kReplReplConf, std::memory_order_relaxed);
LOG(INFO) << "[replication] replconf request was sent, waiting for response";
return CBState::NEXT;
}
ReplicationThread::CBState ReplicationThread::replConfReadCB(bufferevent *bev, void *ctx) {
+ auto self = static_cast<ReplicationThread *>(ctx);
auto input = bufferevent_get_input(bev);
UniqueEvbufReadln line(input, EVBUFFER_EOL_CRLF_STRICT);
if (!line) return CBState::AGAIN;
+ // on unknown option: first try without announce ip, if it fails again - do nothing (to prevent infinite loop)
+ if (line[0] == '-' && isUnknownOption(line.get()) && !self->next_try_without_announce_ip_address_) {
Review Comment:
`line[0] == '-'` seems useless here. (`operator==` in string does the same thing)
--
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] paragor commented on a diff in pull request #1303: Support replica-announce-ip and replica-announce-port
Posted by "paragor (via GitHub)" <gi...@apache.org>.
paragor commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130633132
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,9 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
Review Comment:
done
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,9 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
+ std::string GetAnnounceIP() { return announce_ip_; }
Review Comment:
done
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#issuecomment-1461403629
Thanks for your contribution, this's a very useful PR.
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130611545
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,9 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
+ std::string GetAnnounceIP() { return announce_ip_; }
+ std::string GetAnnounceIPOrIP() { return !announce_ip_.empty() ? announce_ip_ : ip_; }
Review Comment:
GetAnnounceIP is only meaning to slave connections and others should also use GetIP, so we need to keep the GetIP function.
--
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] paragor commented on a diff in pull request #1303: Support replica-announce-ip and replica-announce-port
Posted by "paragor (via GitHub)" <gi...@apache.org>.
paragor commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130843831
##########
src/cluster/replication.cc:
##########
@@ -425,18 +425,35 @@ ReplicationThread::CBState ReplicationThread::checkDBNameReadCB(bufferevent *bev
ReplicationThread::CBState ReplicationThread::replConfWriteCB(bufferevent *bev, void *ctx) {
auto self = static_cast<ReplicationThread *>(ctx);
- send_string(bev,
- Redis::MultiBulkString({"replconf", "listening-port", std::to_string(self->srv_->GetConfig()->port)}));
+ auto config = self->srv_->GetConfig();
+
+ auto port = config->replica_announce_port > 0 ? config->replica_announce_port : config->port;
+ if (!self->next_try_without_announce_ip_address_ && !config->replica_announce_ip.empty()) {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port), "ip-address",
+ config->replica_announce_ip}));
+ } else {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port)}));
+ }
Review Comment:
done
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130657170
##########
src/cluster/replication.cc:
##########
@@ -425,18 +425,35 @@ ReplicationThread::CBState ReplicationThread::checkDBNameReadCB(bufferevent *bev
ReplicationThread::CBState ReplicationThread::replConfWriteCB(bufferevent *bev, void *ctx) {
auto self = static_cast<ReplicationThread *>(ctx);
- send_string(bev,
- Redis::MultiBulkString({"replconf", "listening-port", std::to_string(self->srv_->GetConfig()->port)}));
+ auto config = self->srv_->GetConfig();
+
+ auto port = config->replica_announce_port > 0 ? config->replica_announce_port : config->port;
+ if (!self->next_try_without_announce_ip_address_ && !config->replica_announce_ip.empty()) {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port), "ip-address",
+ config->replica_announce_ip}));
+ } else {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port)}));
+ }
Review Comment:
```suggestion
std::vector<std::string> data_to_send{"replconf", "listening-port", std::to_string(port)};
if (!self->next_try_without_announce_ip_address_ && !config->replica_announce_ip.empty()) {
data_to_send.push_back("ip-address");
data_to_send.push_back(config->replica_announce_ip);
}
send_string(bev, Redis::MultiBulkString());
```
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130577542
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,9 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
+ std::string GetAnnounceIP() { return announce_ip_; }
Review Comment:
There seems to be no place to use `GetAnnounceIP()`, and I personally don't feel the need to add this function
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,9 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
+ std::string GetAnnounceIP() { return announce_ip_; }
+ std::string GetAnnounceIPOrIP() { return !announce_ip_.empty() ? announce_ip_ : ip_; }
Review Comment:
We can do this directly in the `GetIP()` function. If you want a different name, delete `GetIP()` which not used anywhere else.
##########
src/cluster/replication.cc:
##########
@@ -425,18 +425,35 @@ ReplicationThread::CBState ReplicationThread::checkDBNameReadCB(bufferevent *bev
ReplicationThread::CBState ReplicationThread::replConfWriteCB(bufferevent *bev, void *ctx) {
auto self = static_cast<ReplicationThread *>(ctx);
- send_string(bev,
- Redis::MultiBulkString({"replconf", "listening-port", std::to_string(self->srv_->GetConfig()->port)}));
+ auto config = self->srv_->GetConfig();
+
+ auto port = config->replica_announce_port > 0 ? config->replica_announce_port : config->port;
+ if (!self->next_try_without_announce_ip_address_ && !config->replica_announce_ip.empty()) {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port), "ip-address",
+ config->replica_announce_ip}));
+ } else {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port)}));
Review Comment:
Personally, I think use `vector` more concise.
--
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] paragor commented on a diff in pull request #1303: Support replica-announce-ip and replica-announce-port
Posted by "paragor (via GitHub)" <gi...@apache.org>.
paragor commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130599593
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,9 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
Review Comment:
i think i forgot to add opportunity to kill client via announce addr
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130651921
##########
src/server/server.cc:
##########
@@ -947,7 +947,7 @@ void Server::GetRoleInfo(std::string *info) {
if (slave->IsStopped()) continue;
list.emplace_back(Redis::MultiBulkString({
- slave->GetConn()->GetIP(),
+ slave->GetConn()->GetAnnounceIPOrIP(),
std::to_string(slave->GetConn()->GetListeningPort()),
Review Comment:
ditto
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130611845
##########
src/cluster/replication.cc:
##########
@@ -425,18 +425,35 @@ ReplicationThread::CBState ReplicationThread::checkDBNameReadCB(bufferevent *bev
ReplicationThread::CBState ReplicationThread::replConfWriteCB(bufferevent *bev, void *ctx) {
auto self = static_cast<ReplicationThread *>(ctx);
- send_string(bev,
- Redis::MultiBulkString({"replconf", "listening-port", std::to_string(self->srv_->GetConfig()->port)}));
+ auto config = self->srv_->GetConfig();
+
+ auto port = config->replica_announce_port > 0 ? config->replica_announce_port : config->port;
+ if (!self->next_try_without_announce_ip_address_ && !config->replica_announce_ip.empty()) {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port), "ip-address",
+ config->replica_announce_ip}));
+ } else {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port)}));
Review Comment:
We repeat `'replconf", "listening-port", std::to_string(port)}`, `send_string()`, and we have other options in the future that we can append more concisely.
--
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] paragor commented on a diff in pull request #1303: Support replica-announce-ip and replica-announce-port
Posted by "paragor (via GitHub)" <gi...@apache.org>.
paragor commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130627894
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,9 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
Review Comment:
yeah, i add it, wait local tests pass and then will push
--
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] paragor commented on a diff in pull request #1303: Support replica-announce-ip and replica-announce-port
Posted by "paragor (via GitHub)" <gi...@apache.org>.
paragor commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130839750
##########
src/server/server.cc:
##########
@@ -910,7 +910,7 @@ void Server::GetReplicationInfo(std::string *info) {
if (slave->IsStopped()) continue;
string_stream << "slave" << std::to_string(idx) << ":";
- string_stream << "ip=" << slave->GetConn()->GetIP() << ",port=" << slave->GetConn()->GetListeningPort()
+ string_stream << "ip=" << slave->GetConn()->GetAnnounceIPOrIP() << ",port=" << slave->GetConn()->GetListeningPort()
Review Comment:
GetListeningPort contains announce-port
see function `ReplicationThread::CBState ReplicationThread::replConfWriteCB`
##########
src/server/server.cc:
##########
@@ -910,7 +910,7 @@ void Server::GetReplicationInfo(std::string *info) {
if (slave->IsStopped()) continue;
string_stream << "slave" << std::to_string(idx) << ":";
- string_stream << "ip=" << slave->GetConn()->GetIP() << ",port=" << slave->GetConn()->GetListeningPort()
+ string_stream << "ip=" << slave->GetConn()->GetAnnounceIPOrIP() << ",port=" << slave->GetConn()->GetListeningPort()
Review Comment:
GetListeningPort contains announce-port
see function `ReplicationThread::CBState ReplicationThread::replConfWriteCB(bufferevent *bev, void *ctx) {`
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130640043
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,11 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
+ std::string GetAnnounceIPOrIP() { return !announce_ip_.empty() ? announce_ip_ : ip_; }
Review Comment:
I prefer just `GetAnnounceIP` here.
##########
src/cluster/replication.cc:
##########
@@ -425,18 +425,35 @@ ReplicationThread::CBState ReplicationThread::checkDBNameReadCB(bufferevent *bev
ReplicationThread::CBState ReplicationThread::replConfWriteCB(bufferevent *bev, void *ctx) {
auto self = static_cast<ReplicationThread *>(ctx);
- send_string(bev,
- Redis::MultiBulkString({"replconf", "listening-port", std::to_string(self->srv_->GetConfig()->port)}));
+ auto config = self->srv_->GetConfig();
+
+ auto port = config->replica_announce_port > 0 ? config->replica_announce_port : config->port;
+ if (!self->next_try_without_announce_ip_address_ && !config->replica_announce_ip.empty()) {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port), "ip-address",
+ config->replica_announce_ip}));
+ } else {
+ send_string(bev, Redis::MultiBulkString({"replconf", "listening-port", std::to_string(port)}));
+ }
+
self->repl_state_.store(kReplReplConf, std::memory_order_relaxed);
LOG(INFO) << "[replication] replconf request was sent, waiting for response";
return CBState::NEXT;
}
ReplicationThread::CBState ReplicationThread::replConfReadCB(bufferevent *bev, void *ctx) {
+ auto self = static_cast<ReplicationThread *>(ctx);
auto input = bufferevent_get_input(bev);
UniqueEvbufReadln line(input, EVBUFFER_EOL_CRLF_STRICT);
if (!line) return CBState::AGAIN;
+ // on unknown option: first try without announce ip, if it fails again - do nothing (to prevent infinite loop)
+ if (line[0] == '-' && isUnknownOption(line.get()) && !self->next_try_without_announce_ip_address_) {
Review Comment:
`line[0] == '-'` seems useless here. (`operator==` in string does sthe ame thing)
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,11 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
+ std::string GetAnnounceIPOrIP() { return !announce_ip_.empty() ? announce_ip_ : ip_; }
+ std::string GetAnnounceAddr() {
+ return GetAnnounceIPOrIP() + ":" + std::to_string(listening_port_ > 0 ? listening_port_ : port_);
Review Comment:
AFAIK `port_` is the peer of `listening_port_`, so I can hardly find the meaning of `listening_port_ > 0 ? listening_port_ : port_`. Could you make some hints to me?
##########
src/server/server.cc:
##########
@@ -910,7 +910,7 @@ void Server::GetReplicationInfo(std::string *info) {
if (slave->IsStopped()) continue;
string_stream << "slave" << std::to_string(idx) << ":";
- string_stream << "ip=" << slave->GetConn()->GetIP() << ",port=" << slave->GetConn()->GetListeningPort()
+ string_stream << "ip=" << slave->GetConn()->GetAnnounceIPOrIP() << ",port=" << slave->GetConn()->GetListeningPort()
Review Comment:
Here seems we use `slave->GetConn()->GetListeningPort()` instead of the announce-port, is it right?
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#issuecomment-1462117438
Thanks for your contribution and patient again. @paragor
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#issuecomment-1462115882
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] git-hulk merged pull request #1303: Support replica-announce-ip and replica-announce-port
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303
--
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] paragor commented on a diff in pull request #1303: Support replica-announce-ip and replica-announce-port
Posted by "paragor (via GitHub)" <gi...@apache.org>.
paragor commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130627894
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,9 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
Review Comment:
yeah, i add it, wait local tests pass
--
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 #1303: Support replica-announce-ip and replica-announce-port
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #1303:
URL: https://github.com/apache/incubator-kvrocks/pull/1303#discussion_r1130614219
##########
src/server/redis_connection.h:
##########
@@ -90,6 +90,9 @@ class Connection {
uint32_t GetPort() { return port_; }
void SetListeningPort(int port) { listening_port_ = port; }
int GetListeningPort() { return listening_port_; }
+ void SetAnnounceIP(std::string ip) { announce_ip_ = std::move(ip); }
+ std::string GetAnnounceIP() { return announce_ip_; }
+ std::string GetAnnounceIPOrIP() { return !announce_ip_.empty() ? announce_ip_ : ip_; }
Review Comment:
There is no problem keeping this, individuals prefer when to use and when to add.
--
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