You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "sryanyuan (via GitHub)" <gi...@apache.org> on 2024/11/14 09:41:48 UTC
[PR] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
sryanyuan opened a new pull request, #2662:
URL: https://github.com/apache/kvrocks/pull/2662
If the master is lost, the replication thread will block until the keepalive timer is reached when receiving full-sync SST files. At the same time, if we execute a 'clusterx setnodes' command, it will hold an exclusive lock until the replication thread is stopped. This will cause all other worker threads to block.
My solution is to enable the socket read timeout on the file descriptor receiving SST files, if a timeout occurs and the replication thread is marked as stopped, the receiving action will be broken.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#issuecomment-2480539648
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2662) **Quality Gate failed**
Failed conditions
 [37.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2662&metric=new_coverage&view=list) (required ≥ 50%)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2662)
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843975784
##########
src/common/io_util.cc:
##########
@@ -502,7 +502,11 @@ StatusOr<int> EvbufferRead(evbuffer *buf, evutil_socket_t fd, int howmuch, [[may
howmuch = BUFFER_SIZE;
}
if (howmuch = SSL_read(ssl, tmp, howmuch); howmuch <= 0) {
- return {Status::NotOK, fmt::format("failed to read from SSL connection: {}", fmt::streamed(SSLError(howmuch)))};
+ if (howmuch == 0) {
+ return {Status::EndOfFile, "EOF encountered while reading from SSL connection"};
+ }
+ return {(errno == EWOULDBLOCK || errno == EAGAIN) ? Status::TryAgain : Status::NotOK,
+ fmt::format("failed to read from SSL connection: {}", fmt::streamed(SSLError(howmuch)))};
Review Comment:
```suggestion
int err = SSL_get_error(ssl, howmuch);
if (err == SSL_ERROR_ZERO_RETURN) {
return {Status::EndOfFile, "EOF encountered while reading from SSL connection"};
}
return {(err == SSL_ERROR_WANT_READ) ? Status::TryAgain : Status::NotOK,
fmt::format("failed to read from SSL connection: {}", fmt::streamed(SSLError(howmuch)))};
```
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843671829
##########
src/common/io_util.cc:
##########
@@ -515,7 +515,8 @@ StatusOr<int> EvbufferRead(evbuffer *buf, evutil_socket_t fd, int howmuch, [[may
if (int ret = evbuffer_read(buf, fd, howmuch); ret > 0) {
return ret;
} else {
- return {Status::NotOK, fmt::format("failed to read from socket: {}", strerror(errno))};
+ return ret == 0 ? Status{Status::NetworkEOF, "EOF"}
+ : Status{Status::NotOK, fmt::format("failed to read from socket: {}", strerror(errno))};
}
Review Comment:
```c++
if (int ret = evbuffer_read(buf, fd, howmuch); ret > 0) {
return ret;
} else if (ret == 0) {
return {Status::EndOfFile, "EOF encountered while reading from socket"};
} else {
return {Status::NotOK, fmt::format("failed to read from socket: {}", strerror(errno))};
}
```
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1842008161
##########
src/cluster/replication.cc:
##########
@@ -874,6 +876,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ if (stop_flag_) {
+ return {Status::NotOK, "replication thread was stopped"};
+ }
+ continue;
+ }
Review Comment:
The other errors (not timeout), just returned as before
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843138763
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
Review Comment:
> Yeah we can rename it with a `-ms` suffix.
ok
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843159914
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
+
+# Change the default timeout in milliseconds for socket recv during fullsync.
+# The default value is 3200, and 0 means no timeout.
+#
+# If the master is unreachable when fetching SST files, not having a timeout may block
+# future 'clusterx setnodes' commands because the replication thread is blocked on recv.
+slave-fullsync-recv-timeout 3200
Review Comment:
> There's also another socket connection in ReplicationThread::CallbacksStateMachine. Could we unify these connection timeout settings in replication and name this config option replication-..-timeout? cc @git-hulk
Yes, it would be better to use a unify connection timeout for replication sockets.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#issuecomment-2480492791
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2662) **Quality Gate failed**
Failed conditions
 [32.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2662&metric=new_coverage&view=list) (required ≥ 50%)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2662)
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#issuecomment-2480586769
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2662) **Quality Gate failed**
Failed conditions
 [37.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2662&metric=new_coverage&view=list) (required ≥ 50%)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2662)
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843082075
##########
src/cluster/replication.cc:
##########
@@ -770,14 +770,16 @@ Status ReplicationThread::parallelFetchFile(const std::string &dir,
if (this->stop_flag_) {
return {Status::NotOK, "replication thread was stopped"};
}
+ int sock_timeout_ms = 3000;
ssl_st *ssl = nullptr;
#ifdef ENABLE_OPENSSL
if (this->srv_->GetConfig()->tls_replication) {
ssl = SSL_new(this->srv_->ssl_ctx.get());
}
auto exit = MakeScopeExit([ssl] { SSL_free(ssl); });
#endif
- int sock_fd = GET_OR_RET(util::SockConnect(this->host_, this->port_, ssl).Prefixed("connect the server err"));
+ int sock_fd = GET_OR_RET(util::SockConnect(this->host_, this->port_, ssl, sock_timeout_ms, sock_timeout_ms)
Review Comment:
> I suggest separating those two default values due to their backoff interval is different. For the TCP `syn` packet loss retry in Linux is `1s`, `2s`, `4s`, `8s`, so a value like `3200ms` will be better than `3000ms` because it would retry twice if it's loss due to high traffic load.
>
> And for the read/write backoff is related to RTO, but it's generally around 200ms in the internal network, so its backoff would be `200ms`, `400ms`, `800ms`, `1600ms`, so it will be better to set it to a value like `3100ms`.
updated
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843136056
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
Review Comment:
Nit: we need to denote it's microsec / millsecs or other
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
Review Comment:
Nit: we need to denote it's microsec / millsecs or other in comment
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843137008
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
Review Comment:
Yeah we can rename it with a `-ms` suffix.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843159034
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
+
+# Change the default timeout in milliseconds for socket recv during fullsync.
+# The default value is 3200, and 0 means no timeout.
+#
+# If the master is unreachable when fetching SST files, not having a timeout may block
+# future 'clusterx setnodes' commands because the replication thread is blocked on recv.
+slave-fullsync-recv-timeout 3200
Review Comment:
The main replication thread is also using a blocking socket for receiving data, so it may block if the master is unreachable. I will test this case and modify it if the situation exists.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843708814
##########
src/cluster/replication.cc:
##########
@@ -874,6 +876,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (s.GetCode() != Status::NetworkEOF && (errno == EAGAIN || errno == EWOULDBLOCK)) {
Review Comment:
```suggestion
if (s.Is<Status::TryAgain>())) {
```
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843230691
##########
src/cluster/replication.cc:
##########
@@ -874,6 +877,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
Review Comment:
We should check the timeout error because if the master sends SST files with low rate, then it can sleep a long time to send the next SST file.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843671829
##########
src/common/io_util.cc:
##########
@@ -515,7 +515,8 @@ StatusOr<int> EvbufferRead(evbuffer *buf, evutil_socket_t fd, int howmuch, [[may
if (int ret = evbuffer_read(buf, fd, howmuch); ret > 0) {
return ret;
} else {
- return {Status::NotOK, fmt::format("failed to read from socket: {}", strerror(errno))};
+ return ret == 0 ? Status{Status::NetworkEOF, "EOF"}
+ : Status{Status::NotOK, fmt::format("failed to read from socket: {}", strerror(errno))};
}
Review Comment:
```c++
if (int ret = evbuffer_read(buf, fd, howmuch); ret > 0) {
return ret;
} else if (ret == 0) {
return {Status::EndOfFile, "EOF encountered while reading from socket"};
} else {
return {(error == EAGAIN || error == EWOULDBLOCK ? Status::TryAgain, Status::NotOK),
fmt::format("failed to read from socket: {}", strerror(errno))};
}
```
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1841900461
##########
src/cluster/replication.cc:
##########
@@ -770,14 +770,16 @@ Status ReplicationThread::parallelFetchFile(const std::string &dir,
if (this->stop_flag_) {
return {Status::NotOK, "replication thread was stopped"};
}
+ int sock_timeout_ms = 3000;
Review Comment:
ok, updated
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843124580
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
+
+# Change the default timeout in milliseconds for socket recv during fullsync.
+# The default value is 3200, and 0 means no timeout.
+#
+# If the master is unreachable when fetching SST files, not having a timeout may block
+# future 'clusterx setnodes' commands because the replication thread is blocked on recv.
+slave-fullsync-recv-timeout 3200
Review Comment:
There's also another socket connection in `ReplicationThread::CallbacksStateMachine`. Could we unify these connection timeout settings in replication and name this config option `replication-..-timeout`? cc @git-hulk
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1844961822
##########
src/config/config.h:
##########
@@ -105,6 +105,8 @@ struct Config {
bool slave_serve_stale_data = true;
bool slave_empty_db_before_fullsync = false;
int slave_priority = 100;
+ int replication_connect_timeout_ms = 3200;
+ int replication_recv_timeout_ms = 3100;
Review Comment:
```suggestion
int replication_connect_timeout_ms = 3100;
int replication_recv_timeout_ms = 3200;
```
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice merged PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843124029
##########
src/cluster/replication.cc:
##########
@@ -770,14 +770,16 @@ Status ReplicationThread::parallelFetchFile(const std::string &dir,
if (this->stop_flag_) {
return {Status::NotOK, "replication thread was stopped"};
}
+ int sock_timeout_ms = 3000;
ssl_st *ssl = nullptr;
#ifdef ENABLE_OPENSSL
if (this->srv_->GetConfig()->tls_replication) {
ssl = SSL_new(this->srv_->ssl_ctx.get());
}
auto exit = MakeScopeExit([ssl] { SSL_free(ssl); });
#endif
- int sock_fd = GET_OR_RET(util::SockConnect(this->host_, this->port_, ssl).Prefixed("connect the server err"));
+ int sock_fd = GET_OR_RET(util::SockConnect(this->host_, this->port_, ssl, sock_timeout_ms, sock_timeout_ms)
Review Comment:
> * Why have the connection timeout and the timeout both been set to the same value?
> * Also why is the timeout set to 3000 here?
> * Should we make this configurable?
I will make timeout config fields configurable, and using the default value suggested by hulk :)
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843672517
##########
src/common/io_util.cc:
##########
@@ -515,7 +515,8 @@ StatusOr<int> EvbufferRead(evbuffer *buf, evutil_socket_t fd, int howmuch, [[may
if (int ret = evbuffer_read(buf, fd, howmuch); ret > 0) {
Review Comment:
Also we need to add it for the SSL part.
##########
src/common/io_util.cc:
##########
@@ -515,7 +515,8 @@ StatusOr<int> EvbufferRead(evbuffer *buf, evutil_socket_t fd, int howmuch, [[may
if (int ret = evbuffer_read(buf, fd, howmuch); ret > 0) {
Review Comment:
Also we need to add it for the SSL part above.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1842008161
##########
src/cluster/replication.cc:
##########
@@ -874,6 +876,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ if (stop_flag_) {
+ return {Status::NotOK, "replication thread was stopped"};
+ }
+ continue;
+ }
Review Comment:
The other errors (not timeout), just returned as previous
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1842008161
##########
src/cluster/replication.cc:
##########
@@ -874,6 +876,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ if (stop_flag_) {
+ return {Status::NotOK, "replication thread was stopped"};
+ }
+ continue;
+ }
Review Comment:
The other errors (not timeout), just returned as before
From man page:
SO_RCVTIMEO and SO_SNDTIMEO
Specify the receiving or sending timeouts until reporting an error. The argument is a struct timeval. If an input or output function blocks for this period of time, and data has been sent or received, the return value of that function will be the amount of data transferred; **if no data has been transferred and the timeout has been reached then -1 is returned with errno set to EAGAIN or EWOULDBLOCK, or EINPROGRESS (for [connect](https://linux.die.net/man/2/connect)(2)) just as if the socket was specified to be nonblocking.** If the timeout is set to zero (the default) then the operation will never timeout. Timeouts only have effect for system calls that perform socket I/O (e.g., [read](https://linux.die.net/man/2/read)(2), [recvmsg](https://linux.die.net/man/2/recvmsg)(2), [send](https://linux.die.net/man/2/send)(2), [sendmsg](https://linux.die.net/man/2/sendmsg)(2)); timeouts have no effect for [select](https://linux.die.net/man/2/select)(2), [poll](https://linux.die.net/man/2/
poll)(2), [epoll_wait](https://linux.die.net/man/2/epoll_wait)(2), and so on.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#issuecomment-2477960325
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2662) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2662&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2662&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2662&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [51.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2662&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2662&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2662)
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843671829
##########
src/common/io_util.cc:
##########
@@ -515,7 +515,8 @@ StatusOr<int> EvbufferRead(evbuffer *buf, evutil_socket_t fd, int howmuch, [[may
if (int ret = evbuffer_read(buf, fd, howmuch); ret > 0) {
return ret;
} else {
- return {Status::NotOK, fmt::format("failed to read from socket: {}", strerror(errno))};
+ return ret == 0 ? Status{Status::NetworkEOF, "EOF"}
+ : Status{Status::NotOK, fmt::format("failed to read from socket: {}", strerror(errno))};
}
Review Comment:
```c++
if (int ret = evbuffer_read(buf, fd, howmuch); ret > 0) {
return ret;
} else if (ret == 0) {
return {Status::EndOfFile, "EOF encountered while reading from socket"};
} else {
return {(errno == EAGAIN || errno == EWOULDBLOCK ? Status::TryAgain, Status::NotOK),
fmt::format("failed to read from socket: {}", strerror(errno))};
}
```
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843137008
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
Review Comment:
Yeah we can rename it with a `ms` suffix.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843159034
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
+
+# Change the default timeout in milliseconds for socket recv during fullsync.
+# The default value is 3200, and 0 means no timeout.
+#
+# If the master is unreachable when fetching SST files, not having a timeout may block
+# future 'clusterx setnodes' commands because the replication thread is blocked on recv.
+slave-fullsync-recv-timeout 3200
Review Comment:
The main replication thread is also using a blocking socket for receiving data, so it may block if the master is unreachable. I will test this case and modify it if the situation exists.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#issuecomment-2478326143
We can't distinguish whether the result is EOF or error if EvbufferRead returns an error. When the underlying I/O syscall returns EOF, the errno will not be set. So, I added a new EOF status to break the loop if the connection is EOF.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1841964954
##########
src/cluster/replication.cc:
##########
@@ -770,14 +770,16 @@ Status ReplicationThread::parallelFetchFile(const std::string &dir,
if (this->stop_flag_) {
return {Status::NotOK, "replication thread was stopped"};
}
+ int sock_timeout_ms = 3000;
ssl_st *ssl = nullptr;
#ifdef ENABLE_OPENSSL
if (this->srv_->GetConfig()->tls_replication) {
ssl = SSL_new(this->srv_->ssl_ctx.get());
}
auto exit = MakeScopeExit([ssl] { SSL_free(ssl); });
#endif
- int sock_fd = GET_OR_RET(util::SockConnect(this->host_, this->port_, ssl).Prefixed("connect the server err"));
+ int sock_fd = GET_OR_RET(util::SockConnect(this->host_, this->port_, ssl, sock_timeout_ms, sock_timeout_ms)
Review Comment:
I suggest separating those two default values due to their backoff interval is different. For the TCP `syn` packet loss retry in Linux is `1s`, `2s`, `4s`, `8s`, so a value like `3200ms` will be better than `3000ms` because it would retry twice if it's loss due to high traffic load.
And for the read/write backoff is related to RTO, but it's generally around 200ms in the internal network, so its backoff would be `200ms`, `400ms`, `800ms`, `1600ms`, so it will be better to set it to a value like `3100ms`.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1842008161
##########
src/cluster/replication.cc:
##########
@@ -874,6 +876,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ if (stop_flag_) {
+ return {Status::NotOK, "replication thread was stopped"};
+ }
+ continue;
+ }
Review Comment:
The other errors (not timeout), just returned as before
From man page:
SO_RCVTIMEO and SO_SNDTIMEO
Specify the receiving or sending timeouts until reporting an error. The argument is a struct timeval. If an input or output function blocks for this period of time, and data has been sent or received, the return value of that function will be the amount of data transferred; if no data has been transferred and the timeout has been reached then -1 is returned with errno set to EAGAIN or EWOULDBLOCK, or EINPROGRESS (for [connect](https://linux.die.net/man/2/connect)(2)) just as if the socket was specified to be nonblocking. If the timeout is set to zero (the default) then the operation will never timeout. Timeouts only have effect for system calls that perform socket I/O (e.g., [read](https://linux.die.net/man/2/read)(2), [recvmsg](https://linux.die.net/man/2/recvmsg)(2), [send](https://linux.die.net/man/2/send)(2), [sendmsg](https://linux.die.net/man/2/sendmsg)(2)); timeouts have no effect for [select](https://linux.die.net/man/2/select)(2), [poll](https://linux.die.net/man/2/poll)(
2), [epoll_wait](https://linux.die.net/man/2/epoll_wait)(2), and so on.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843079984
##########
src/cluster/replication.cc:
##########
@@ -874,6 +876,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ if (stop_flag_) {
+ return {Status::NotOK, "replication thread was stopped"};
+ }
+ continue;
+ }
Review Comment:
To enter this block, evbuffer_read should returns a non-positive integer to indicate the operation is not successful, so it only enters when the read operation fails.
If it fails, check the errno then we can retry the operation if timed out or quit if replication thread is stopped.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1841964954
##########
src/cluster/replication.cc:
##########
@@ -770,14 +770,16 @@ Status ReplicationThread::parallelFetchFile(const std::string &dir,
if (this->stop_flag_) {
return {Status::NotOK, "replication thread was stopped"};
}
+ int sock_timeout_ms = 3000;
ssl_st *ssl = nullptr;
#ifdef ENABLE_OPENSSL
if (this->srv_->GetConfig()->tls_replication) {
ssl = SSL_new(this->srv_->ssl_ctx.get());
}
auto exit = MakeScopeExit([ssl] { SSL_free(ssl); });
#endif
- int sock_fd = GET_OR_RET(util::SockConnect(this->host_, this->port_, ssl).Prefixed("connect the server err"));
+ int sock_fd = GET_OR_RET(util::SockConnect(this->host_, this->port_, ssl, sock_timeout_ms, sock_timeout_ms)
Review Comment:
I suggest separating those two default values due to their backoff interval is different. For the TCP `syn` packet loss retry in Linux is `1s`, `2s`, `4s`, `8s`, so a value like `3200ms` will be better than `3000ms` because it would retry twice if it's loss due to high traffic load.
And for the read/write backoff is related to RTO, but it's generally around 200ms in the internal network, so its backoff would be `200ms`, `400ms`, `800ms`, `1600ms`, and it will be better to set it to a value like `3100ms`.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843668758
##########
src/common/status.h:
##########
@@ -75,6 +75,9 @@ class [[nodiscard]] Status {
// Search
NoPrefixMatched,
TypeMismatched,
+
+ // Network
+ NetworkEOF,
Review Comment:
```suggestion
EndOfFile,
TryAgain,
```
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1844957667
##########
src/config/config.h:
##########
@@ -105,6 +105,8 @@ struct Config {
bool slave_serve_stale_data = true;
bool slave_empty_db_before_fullsync = false;
int slave_priority = 100;
+ int replication_connect_timeout_ms = 3200;
Review Comment:
Connect is 3100 and receive is 3200. It may not be a big issue here but for convenience.
BTW, you can make them constants since you repeated them twice (and made an error here).
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843128840
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
+
+# Change the default timeout in milliseconds for socket recv during fullsync.
+# The default value is 3200, and 0 means no timeout.
+#
+# If the master is unreachable when fetching SST files, not having a timeout may block
+# future 'clusterx setnodes' commands because the replication thread is blocked on recv.
+slave-fullsync-recv-timeout 3200
Review Comment:
For connection timeout I think it's good to share the config. Not sure if it's fine for read/write timeout. WDYT?
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843201613
##########
src/cluster/replication.cc:
##########
@@ -874,6 +877,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
Review Comment:
This check should be unnecessary since the replication socket is in blocking mode, and it will never return the EAGAIN which is for the non-blocking socket.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#issuecomment-2477964921
By the way, can we add a test with master removed and about 5s' connection timeout?
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843160760
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
+
+# Change the default timeout in milliseconds for socket recv during fullsync.
+# The default value is 3200, and 0 means no timeout.
+#
+# If the master is unreachable when fetching SST files, not having a timeout may block
+# future 'clusterx setnodes' commands because the replication thread is blocked on recv.
+slave-fullsync-recv-timeout 3200
Review Comment:
The main replication thread seems to be using a blocking socket wrapped by bufferevent for receiving data, so it may block if master is unreachable. I will test this case and modify it if the situation exists.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1841959706
##########
src/cluster/replication.cc:
##########
@@ -874,6 +876,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ if (stop_flag_) {
+ return {Status::NotOK, "replication thread was stopped"};
+ }
+ continue;
+ }
Review Comment:
Our QA team tested and it do resolve the issue (On X86-64 centos).
It depends on the return value of evbuffer_read, if it returns 0, it means the connection is EOF, or -1 if error occurred. So timeout will always return Status with NotOK and errno will be EAGAIN or EWOULDBLOCK as manual says:
if no data has been transferred and the timeout has been reached then -1 is returned with errno set to EAGAIN or EWOULDBLOCK, or EINPROGRESS (for [connect](https://linux.die.net/man/2/connect)(2)) just as if the socket was specified to be nonblocking.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1842008161
##########
src/cluster/replication.cc:
##########
@@ -874,6 +876,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ if (stop_flag_) {
+ return {Status::NotOK, "replication thread was stopped"};
+ }
+ continue;
+ }
Review Comment:
The other errors (not timeout), just returned as before
From man page:
SO_RCVTIMEO and SO_SNDTIMEO
Specify the receiving or sending timeouts until reporting an error. The argument is a struct timeval. If an input or output function blocks for this period of time, and data has been sent or received, the return value of that function will be the amount of data transferred; **if no data has been transferred and the timeout has been reached then -1 is returned with errno set to EAGAIN or EWOULDBLOCK, or EINPROGRESS (for [connect](https://linux.die.net/man/2/connect)(2)) just as if the socket was specified to be nonblocking.** If the timeout is set to zero (the default) then the operation will never timeout. Timeouts only have effect for system calls that perform socket I/O (e.g., [read](https://linux.die.net/man/2/read)(2), [recvmsg](https://linux.die.net/man/2/recvmsg)(2), [send](https://linux.die.net/man/2/send)(2), [sendmsg](https://linux.die.net/man/2/sendmsg)(2)); timeouts have no effect for [select](https://linux.die.net/man/2/select)(2), [poll](https://linux.die.net/man/2/
poll)(2), [epoll_wait](https://linux.die.net/man/2/epoll_wait)(2), and so on.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843228628
##########
src/cluster/replication.cc:
##########
@@ -874,6 +877,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
Review Comment:
> This check should be unnecessary since the replication socket is in blocking mode, and it will never return the EAGAIN which is for the non-blocking socket.
According to he man page:
SO_RCVTIMEO and SO_SNDTIMEO
Specify the receiving or sending timeouts until reporting an error. The argument is a struct timeval. If an input or output function blocks for this period of time, and data has been sent or received, the return value of that function will be the amount of data transferred; **if no data has been transferred and the timeout has been reached then -1 is returned with errno set to EAGAIN or EWOULDBLOCK, or EINPROGRESS (for [connect](https://linux.die.net/man/2/connect)(2)) just as if the socket was specified to be nonblocking.** If the timeout is set to zero (the default) then the operation will never timeout. Timeouts only have effect for system calls that perform socket I/O (e.g., [read](https://linux.die.net/man/2/read)(2), [recvmsg](https://linux.die.net/man/2/recvmsg)(2), [send](https://linux.die.net/man/2/send)(2), [sendmsg](https://linux.die.net/man/2/sendmsg)(2)); timeouts have no effect for [select](https://linux.die.net/man/2/select)(2), [poll](https://linux.die.net/man/2/po
ll)(2), [epoll_wait](https://linux.die.net/man/2/epoll_wait)(2), and so on.
##########
src/cluster/replication.cc:
##########
@@ -874,6 +877,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
Review Comment:
> This check should be unnecessary since the replication socket is in blocking mode, and it will never return the EAGAIN which is for the non-blocking socket.
According to the man page:
SO_RCVTIMEO and SO_SNDTIMEO
Specify the receiving or sending timeouts until reporting an error. The argument is a struct timeval. If an input or output function blocks for this period of time, and data has been sent or received, the return value of that function will be the amount of data transferred; **if no data has been transferred and the timeout has been reached then -1 is returned with errno set to EAGAIN or EWOULDBLOCK, or EINPROGRESS (for [connect](https://linux.die.net/man/2/connect)(2)) just as if the socket was specified to be nonblocking.** If the timeout is set to zero (the default) then the operation will never timeout. Timeouts only have effect for system calls that perform socket I/O (e.g., [read](https://linux.die.net/man/2/read)(2), [recvmsg](https://linux.die.net/man/2/recvmsg)(2), [send](https://linux.die.net/man/2/send)(2), [sendmsg](https://linux.die.net/man/2/sendmsg)(2)); timeouts have no effect for [select](https://linux.die.net/man/2/select)(2), [poll](https://linux.die.net/man/2/po
ll)(2), [epoll_wait](https://linux.die.net/man/2/epoll_wait)(2), and so on.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843695303
##########
src/common/io_util.cc:
##########
@@ -515,7 +515,8 @@ StatusOr<int> EvbufferRead(evbuffer *buf, evutil_socket_t fd, int howmuch, [[may
if (int ret = evbuffer_read(buf, fd, howmuch); ret > 0) {
Review Comment:
ok, i will modify it later
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#issuecomment-2479412309
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2662) **Quality Gate failed**
Failed conditions
 [27.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2662&metric=new_coverage&view=list) (required ≥ 50%)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2662)
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#issuecomment-2478741558
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2662) **Quality Gate failed**
Failed conditions
 [28.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2662&metric=new_coverage&view=list) (required ≥ 50%)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2662)
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1841885284
##########
src/cluster/replication.cc:
##########
@@ -770,14 +770,16 @@ Status ReplicationThread::parallelFetchFile(const std::string &dir,
if (this->stop_flag_) {
return {Status::NotOK, "replication thread was stopped"};
}
+ int sock_timeout_ms = 3000;
Review Comment:
I think it's better to move it to the top of the file and make it const/constexpr.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1841914539
##########
src/cluster/replication.cc:
##########
@@ -770,14 +770,16 @@ Status ReplicationThread::parallelFetchFile(const std::string &dir,
if (this->stop_flag_) {
return {Status::NotOK, "replication thread was stopped"};
}
+ int sock_timeout_ms = 3000;
ssl_st *ssl = nullptr;
#ifdef ENABLE_OPENSSL
if (this->srv_->GetConfig()->tls_replication) {
ssl = SSL_new(this->srv_->ssl_ctx.get());
}
auto exit = MakeScopeExit([ssl] { SSL_free(ssl); });
#endif
- int sock_fd = GET_OR_RET(util::SockConnect(this->host_, this->port_, ssl).Prefixed("connect the server err"));
+ int sock_fd = GET_OR_RET(util::SockConnect(this->host_, this->port_, ssl, sock_timeout_ms, sock_timeout_ms)
Review Comment:
- Why have the connection timeout and the timeout both been set to the same value?
- Also why is the timeout set to 3000 here?
- Should we make this configurable?
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1841923440
##########
src/cluster/replication.cc:
##########
@@ -874,6 +876,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ if (stop_flag_) {
+ return {Status::NotOK, "replication thread was stopped"};
+ }
+ continue;
+ }
Review Comment:
I think you can notice that to enter this block, the status must be OK (i.e. `if (auto s = ...; !s)`).
It means that, `evbuffer_read` must have a positive integer as a return value to indicate that the operation is successful. So I highly suspect that the errno will not be either of these two.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1841959706
##########
src/cluster/replication.cc:
##########
@@ -874,6 +876,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ if (stop_flag_) {
+ return {Status::NotOK, "replication thread was stopped"};
+ }
+ continue;
+ }
Review Comment:
Our QA team tested and it do resolve the issue (On X86-64 centos).
It depends on the return value of evbuffer_read, if it returns 0, it means the connection is EOF, or -1 if error occurred. So timeout will always return Status with NotOK and errno will be EAGAIN or EWOULDBLOCK as manual says:
if no data has been transferred and the timeout has been reached then -1 is returned with errno set to EAGAIN or EWOULDBLOCK, or EINPROGRESS (for [connect](https://linux.die.net/man/2/connect)(2)) just as if the socket was specified to be nonblocking.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843668758
##########
src/common/status.h:
##########
@@ -75,6 +75,9 @@ class [[nodiscard]] Status {
// Search
NoPrefixMatched,
TypeMismatched,
+
+ // Network
+ NetworkEOF,
Review Comment:
```suggestion
EndOfFile,
```
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843111779
##########
src/cluster/replication.cc:
##########
@@ -874,6 +876,12 @@ Status ReplicationThread::fetchFile(int sock_fd, evbuffer *evbuf, const std::str
UniqueEvbufReadln line(evbuf, EVBUFFER_EOL_CRLF_STRICT);
if (!line) {
if (auto s = util::EvbufferRead(evbuf, sock_fd, -1, ssl); !s) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ if (stop_flag_) {
+ return {Status::NotOK, "replication thread was stopped"};
+ }
+ continue;
+ }
Review Comment:
Ahh you are right, sorry. It looks good to me 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
Re: [PR] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843163384
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
+
+# Change the default timeout in milliseconds for socket recv during fullsync.
+# The default value is 3200, and 0 means no timeout.
+#
+# If the master is unreachable when fetching SST files, not having a timeout may block
+# future 'clusterx setnodes' commands because the replication thread is blocked on recv.
+slave-fullsync-recv-timeout 3200
Review Comment:
The replication socket is a blocking socket, but it won't block forever if we have set the timeout when connecting.
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843124580
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
+
+# Change the default timeout in milliseconds for socket recv during fullsync.
+# The default value is 3200, and 0 means no timeout.
+#
+# If the master is unreachable when fetching SST files, not having a timeout may block
+# future 'clusterx setnodes' commands because the replication thread is blocked on recv.
+slave-fullsync-recv-timeout 3200
Review Comment:
There also another socket connection in `ReplicationThread::CallbacksStateMachine`. Could you unify these connection timeout settings in replication and name this config option `replication-..-timeout`? cc @git-hulk
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
+
+# Change the default timeout in milliseconds for socket recv during fullsync.
+# The default value is 3200, and 0 means no timeout.
+#
+# If the master is unreachable when fetching SST files, not having a timeout may block
+# future 'clusterx setnodes' commands because the replication thread is blocked on recv.
+slave-fullsync-recv-timeout 3200
Review Comment:
There also another socket connection in `ReplicationThread::CallbacksStateMachine`. Could we unify these connection timeout settings in replication and name this config option `replication-..-timeout`? cc @git-hulk
--
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] fix(replication): slave blocks until keepalive timer is reached when master is gone without fin/rst notification [kvrocks]
Posted by "sryanyuan (via GitHub)" <gi...@apache.org>.
sryanyuan commented on code in PR #2662:
URL: https://github.com/apache/kvrocks/pull/2662#discussion_r1843227917
##########
kvrocks.conf:
##########
@@ -174,6 +174,20 @@ slave-read-only yes
# By default the priority is 100.
slave-priority 100
+# Change the default timeout in milliseconds for socket connect during fullsync.
+# The default value is 3100, and 0 means no timeout.
+#
+# If the master is unreachable before connecting, not having a timeout may block future
+# 'clusterx setnodes' commands because the replication thread is blocked on connect.
+slave-fullsync-connect-timeout 3100
+
+# Change the default timeout in milliseconds for socket recv during fullsync.
+# The default value is 3200, and 0 means no timeout.
+#
+# If the master is unreachable when fetching SST files, not having a timeout may block
+# future 'clusterx setnodes' commands because the replication thread is blocked on recv.
+slave-fullsync-recv-timeout 3200
Review Comment:
I unified the connection timeout config. Please have a look.
--
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