You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/12/09 00:43:43 UTC
[GitHub] [incubator-kvrocks] caipengbo opened a new pull request, #1172: Add timeout when replica connect master
caipengbo opened a new pull request, #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172
Fixed: #1170
--
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 #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#discussion_r1044087281
##########
src/cluster/replication.cc:
##########
@@ -236,18 +237,22 @@ void ReplicationThread::CallbacksStateMachine::Start() {
handlers_.emplace_front(CallbacksStateMachine::WRITE, "auth write", authWriteCB);
}
+ uint64_t connect_timestamp = 0, connect_timeout_ms = 3100;
+
while (!repl_->stop_flag_ && bev == nullptr) {
- Status s = Util::SockConnect(repl_->host_, repl_->port_, &cfd);
+ if (Util::GetTimeStampMS() - connect_timestamp < 1000) {
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] caipengbo commented on pull request #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
caipengbo commented on PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#issuecomment-1345792351
Does CI fail because timeout is too short? @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
[GitHub] [incubator-kvrocks] caipengbo commented on a diff in pull request #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#discussion_r1044051983
##########
src/cluster/replication.cc:
##########
@@ -236,18 +237,21 @@ void ReplicationThread::CallbacksStateMachine::Start() {
handlers_.emplace_front(CallbacksStateMachine::WRITE, "auth write", authWriteCB);
}
+ uint64_t connect_timestamp = 0, connect_timeout_ms = 3100;
+
while (!repl_->stop_flag_ && bev == nullptr) {
- Status s = Util::SockConnect(repl_->host_, repl_->port_, &cfd);
+ if (Util::GetTimeStampMS() - connect_timestamp < 1000) {
Review Comment:
I'm terribly sorry, I forgot one line :(
--
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 #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#discussion_r1044061128
##########
src/cluster/replication.cc:
##########
@@ -236,18 +237,22 @@ void ReplicationThread::CallbacksStateMachine::Start() {
handlers_.emplace_front(CallbacksStateMachine::WRITE, "auth write", authWriteCB);
}
+ uint64_t connect_timestamp = 0, connect_timeout_ms = 3100;
+
while (!repl_->stop_flag_ && bev == nullptr) {
- Status s = Util::SockConnect(repl_->host_, repl_->port_, &cfd);
+ if (Util::GetTimeStampMS() - connect_timestamp < 1000) {
Review Comment:
@caipengbo How about renaming connect_timestamp to last_connect_timestamp, and also adding a comment line to explain why we need this logic.
// Sleep here to prevent frequent re-connect when the master is down with the connection refused error
--
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 #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#issuecomment-1346606731
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] PragmaTwice commented on a diff in pull request #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#discussion_r1045615596
##########
src/common/io_util.cc:
##########
@@ -151,9 +151,7 @@ Status SockConnect(const std::string &host, uint32_t port, int *fd, int conn_tim
sin.sin_port = htons(port);
fcntl(*fd, F_SETFL, O_NONBLOCK);
- if (connect(*fd, reinterpret_cast<sockaddr *>(&sin), sizeof(sin))) {
- return Status::FromErrno();
- }
+ connect(*fd, reinterpret_cast<sockaddr *>(&sin), sizeof(sin));
Review Comment:
I think maybe we can filter some specific error codes, e.g.
```cpp
if (int err = connect(...); err && errno != EINPROGRESS)
```
--
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 #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#discussion_r1044051409
##########
src/cluster/replication.cc:
##########
@@ -236,18 +237,21 @@ void ReplicationThread::CallbacksStateMachine::Start() {
handlers_.emplace_front(CallbacksStateMachine::WRITE, "auth write", authWriteCB);
}
+ uint64_t connect_timestamp = 0, connect_timeout_ms = 3100;
+
while (!repl_->stop_flag_ && bev == nullptr) {
- Status s = Util::SockConnect(repl_->host_, repl_->port_, &cfd);
+ if (Util::GetTimeStampMS() - connect_timestamp < 1000) {
Review Comment:
this condition should always be false? coz the connect_timestamp isn't updated at all.
--
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] tanruixiang commented on a diff in pull request #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#discussion_r1045094117
##########
src/cluster/replication.cc:
##########
@@ -236,18 +237,23 @@ void ReplicationThread::CallbacksStateMachine::Start() {
handlers_.emplace_front(CallbacksStateMachine::WRITE, "auth write", authWriteCB);
}
+ uint64_t last_connect_timestamp = 0, connect_timeout_ms = 3100;
Review Comment:
If `connect_timeout_ms` is not dynamically updated, it would be better to use constants?
--
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 #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#issuecomment-1345830652
> Does CI fail because timeout is too short? @git-hulk
Not sure, maybe connecting with timeout didn't work at all, you can have a test on your side.
--
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 #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#discussion_r1044061128
##########
src/cluster/replication.cc:
##########
@@ -236,18 +237,22 @@ void ReplicationThread::CallbacksStateMachine::Start() {
handlers_.emplace_front(CallbacksStateMachine::WRITE, "auth write", authWriteCB);
}
+ uint64_t connect_timestamp = 0, connect_timeout_ms = 3100;
+
while (!repl_->stop_flag_ && bev == nullptr) {
- Status s = Util::SockConnect(repl_->host_, repl_->port_, &cfd);
+ if (Util::GetTimeStampMS() - connect_timestamp < 1000) {
Review Comment:
@caipengbo How about renaming connect_timestamp to last_connect_timestamp, and also adding a comment line to explain why we need this logic.
// Sleep here to prevent frequent re-connect when the master is down with connection refused error
--
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 pull request #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
caipengbo commented on PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#issuecomment-1346104698
I found some problems with `ScopeExit` and `SockConnect(timeout)` @git-hulk @PragmaTwice
--
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 #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#discussion_r1045793757
##########
src/common/io_util.cc:
##########
@@ -151,9 +151,7 @@ Status SockConnect(const std::string &host, uint32_t port, int *fd, int conn_tim
sin.sin_port = htons(port);
fcntl(*fd, F_SETFL, O_NONBLOCK);
- if (connect(*fd, reinterpret_cast<sockaddr *>(&sin), sizeof(sin))) {
- return Status::FromErrno();
- }
+ connect(*fd, reinterpret_cast<sockaddr *>(&sin), sizeof(sin));
Review Comment:
Looks good to me.
--
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 #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#discussion_r1045674333
##########
src/common/io_util.cc:
##########
@@ -151,9 +151,7 @@ Status SockConnect(const std::string &host, uint32_t port, int *fd, int conn_tim
sin.sin_port = htons(port);
fcntl(*fd, F_SETFL, O_NONBLOCK);
- if (connect(*fd, reinterpret_cast<sockaddr *>(&sin), sizeof(sin))) {
- return Status::FromErrno();
- }
+ connect(*fd, reinterpret_cast<sockaddr *>(&sin), sizeof(sin));
Review Comment:
Is it enough for `Util::aeWait()` on the next line to handle the error code?
--
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 closed pull request #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
git-hulk closed pull request #1172: Add timeout when replica connect master
URL: https://github.com/apache/incubator-kvrocks/pull/1172
--
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 #1172: Add timeout when replica connect master
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #1172:
URL: https://github.com/apache/incubator-kvrocks/pull/1172#discussion_r1045094996
##########
src/cluster/replication.cc:
##########
@@ -236,18 +237,23 @@ void ReplicationThread::CallbacksStateMachine::Start() {
handlers_.emplace_front(CallbacksStateMachine::WRITE, "auth write", authWriteCB);
}
+ uint64_t last_connect_timestamp = 0, connect_timeout_ms = 3100;
Review Comment:
I had thought about using a constant before, but found that it wasn't used elsewhere, so I wrote it as a local variable to make it more intuitive.
--
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