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/06/24 11:52:03 UTC
[GitHub] [incubator-kvrocks] tisonkun opened a new pull request, #660: fix: replication tests should wait for server restarted
tisonkun opened a new pull request, #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660
This fixes #656.
This patch is uncompleted. @git-hulk it seems `$stdout` in tcl files doesn't mean stdout from tty. Do you have any background about this test logic?
The issue we should overcome here is to determinately wait for kvrocks server up, instead of sleep 1 second which is unstable.
--
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 #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#issuecomment-1165595780
@tisonkun You're right, we supported to send logs to stdout recently. Need to set `log-dir` to `stdout` on https://github.com/apache/incubator-kvrocks/blob/unstable/tests/tcl/tests/assets/default.conf
--
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 #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#discussion_r906262716
##########
src/server.cc:
##########
@@ -167,6 +167,8 @@ Status Server::Start() {
}
});
+ LOG(INFO) << "Ready to accept connections\n";
Review Comment:
```suggestion
LOG(INFO) << "Ready to accept connections";
```
newline is automatically applied in glog
--
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] tisonkun commented on a diff in pull request #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#discussion_r906110047
##########
src/server.cc:
##########
@@ -167,6 +167,8 @@ Status Server::Start() {
}
});
+ std::cout << "Ready to accept connections\n";
Review Comment:
However TCL tests handle log files hacky and I tend to send this information to stdout now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#issuecomment-1165651100
OK. Let me push a followup commit to switch to `LOG(INFO)` version.
--
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] tisonkun commented on a diff in pull request #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#discussion_r906122155
##########
tests/tcl/tests/support/server.tcl:
##########
@@ -403,7 +403,6 @@ proc start_server {options {code undefined}} {
set unixsocket [file normalize [format "%s/%s" [dict get $config "dir"] "socket"]]
dict set config "unixsocket" $unixsocket
- dict set config "log-dir" [format "%s/%s" [dict get $config "dir"] "stdout"]
Review Comment:
We can redirect log to stdout by setting "log-dir" to "stdout", but we should later update all reference hardcode to `$dir/kvrocks.INFO`
--
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 #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#discussion_r906082997
##########
src/server.cc:
##########
@@ -167,6 +167,8 @@ Status Server::Start() {
}
});
+ std::cout << "Ready to accept connections\n";
Review Comment:
```suggestion
LOG(INFO) << "Ready to accept connections";
```
--
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 #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#discussion_r906082997
##########
src/server.cc:
##########
@@ -167,6 +167,8 @@ Status Server::Start() {
}
});
+ std::cout << "Ready to accept connections\n";
Review Comment:
```suggestion
LOG(INFO) << "Ready to accept connections";
```
Is this what you need?
--
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] tisonkun merged pull request #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
tisonkun merged PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660
--
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] tisonkun commented on pull request #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#issuecomment-1165621537
> @tisonkun You're right, we supported to send logs to stdout recently. Need to set `log-dir` to `stdout` on https://github.com/apache/incubator-kvrocks/blob/unstable/tests/tcl/tests/assets/default.conf
Actually if you search `kvrocks.INFO` we hardcode several file path to the default log path..
I'm sending the "Ready to accept connections" info to stdout for now and see whether we don't suffer from the flaky test. We may later improve stdout and logging logic.
--
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] tisonkun commented on a diff in pull request #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#discussion_r905989166
##########
src/server.cc:
##########
@@ -167,6 +167,8 @@ Status Server::Start() {
}
});
+ std::cout << "Ready to accept connections\n";
Review Comment:
Perhaps we should send this line to log and find a way for tests reading logs.
--
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] tisonkun commented on pull request #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#issuecomment-1165696209
It seems logs are fragmented among files and we should find another way to determinately confirm server has been restarted.
--
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 pull request #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#issuecomment-1165652848
> OK. Let me push a followup commit to switch to `LOG(INFO)` version.
Just address an already-exist problem, so feel free to use stdout. I think we can consider this issue later, and it should not block this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#issuecomment-1165637383
A related problem: I think kvrocks should not output anything on stdout if we set a log file (which is a regular file).
Currently kvrocks still outputs something to stdout when the log file is set, which is confusing.
@git-hulk @tisonkun
--
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] tisonkun commented on pull request #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#issuecomment-1165770817
@git-hulk @PragmaTwice I think this patch is ready to merge. You can give it a review now :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #660: fix: replication tests should wait for server restarted
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #660:
URL: https://github.com/apache/incubator-kvrocks/pull/660#discussion_r906621369
##########
src/server.cc:
##########
@@ -167,6 +167,8 @@ Status Server::Start() {
}
});
+ LOG(INFO) << "Ready to accept connections\n";
Review Comment:
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