[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1303: Support replica-announce-ip and replica-announce-port

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