You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2020/04/01 09:24:38 UTC
[kudu-CR] [postgres] Deflake MiniPostgres tests
Hello Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15629
to review the following change.
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
[postgres] Deflake MiniPostgres tests
Tests depending on MiniPostgres were flaky as in some cases Postgres
takes longer than expected to start. This commit introduces checking if
psql can connect to MiniPostgres as part of start-up and retries the
connection up to 5 times with exponential back-off starting from 100ms.
Ran MiniRangerTest 1000 times on dist-test with no failures with this
fix.
Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
---
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
2 files changed, 30 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/15629/1
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15629
to look at the new patch set (#9).
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
[postgres] Deflake MiniPostgres tests
Tests depending on MiniPostgres were flaky as in some cases Postgres
takes longer than expected to start. This commit introduces checking if
psql can connect to MiniPostgres as part of start-up and retries the
connection up to 5 times with exponential back-off starting from 100ms.
Ran MiniRangerTest 1000 times on dist-test with no failures with this
fix.
Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
---
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
2 files changed, 28 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/15629/9
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Removed -Verified by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.h@88
PS4, Line 88: TestConnection
nit: can you add a comment for this method?
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.cc@92
PS4, Line 92: if (!wait.ok()) {
: // TODO(abukor): implement retry with a different port if it can't bind
: WARN_NOT_OK(process_->Kill(SIGINT), "failed to send SIGINT to Postgres");
: return wait;
: }
RETURN_NOT_OK_PREPEND?
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 17:43:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
[postgres] Deflake MiniPostgres tests
Tests depending on MiniPostgres were flaky as in some cases Postgres
takes longer than expected to start. This commit introduces checking if
psql can connect to MiniPostgres as part of start-up and retries every
100ms for 5 seconds.
Ran MiniRangerTest 1000 times on dist-test with no failures with this
fix.
Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Reviewed-on: http://gerrit.cloudera.org:8080/15629
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
2 files changed, 28 insertions(+), 3 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15629
to look at the new patch set (#5).
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
[postgres] Deflake MiniPostgres tests
Tests depending on MiniPostgres were flaky as in some cases Postgres
takes longer than expected to start. This commit introduces checking if
psql can connect to MiniPostgres as part of start-up and retries the
connection up to 5 times with exponential back-off starting from 100ms.
Ran MiniRangerTest 1000 times on dist-test with no failures with this
fix.
Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
---
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
2 files changed, 33 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/15629/5
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 9: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15629/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15629/9//COMMIT_MSG@11
PS9, Line 11: retries the
: connection up to 5 times with exponential back-off starting from 100ms.
nit: update the commit msg?
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 Apr 2020 16:59:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15629/6/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/15629/6/src/kudu/postgres/mini_postgres.cc@146
PS6, Line 146: TestConnection
nit: maybe rename this to WaitForReady() or something?
http://gerrit.cloudera.org:8080/#/c/15629/6/src/kudu/postgres/mini_postgres.cc@147
PS6, Line 147: FromMilliseconds
nit: can use FromSeconds()
http://gerrit.cloudera.org:8080/#/c/15629/6/src/kudu/postgres/mini_postgres.cc@162
PS6, Line 162: if ((MonoTime::Now() - start) > kTimeout) {
: break;
: }
nit: would be simpler as
MonoTime timeout = Now + delta
Status s;
while (Now < timeout) {
s = psql.Start...Wait()
if (s.ok()) return s;
SleepFor(100ms);
}
return s; // or wrap this in a Status::TimedOut()
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 21:53:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.cc@149
PS4, Line 149: uint8_t i = 0;
: while (true) {
: Subprocess psql({
: JoinPathSegments(bin_dir_, "postgres/pg_isready"),
: "-p", SimpleItoa(port_),
: "-h", host_,
: });
: RETURN_NOT_OK(psql.Start());
: Status s = psql.WaitAndCheckExitCode();
: if (s.ok()) {
: return s;
: }
:
: if (i + 1 >= kRetries) {
: break;
: }
: SleepFor(MonoDelta::FromMilliseconds(100*pow(2,i++)));
: }
nit: why bother with exponential backoff at all? We don't expect much load on Postgres, so it's not like we're trying to find an acceptable rate of calling. How do you feel about a fixed retry interval and a MonoTime deadline instead?
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 20:57:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15629
to look at the new patch set (#2).
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
[postgres] Deflake MiniPostgres tests
Tests depending on MiniPostgres were flaky as in some cases Postgres
takes longer than expected to start. This commit introduces checking if
psql can connect to MiniPostgres as part of start-up and retries the
connection up to 5 times with exponential back-off starting from 100ms.
Ran MiniRangerTest 1000 times on dist-test with no failures with this
fix.
Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
---
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
2 files changed, 32 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/15629/2
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 9: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15629/9/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:
http://gerrit.cloudera.org:8080/#/c/15629/9/src/kudu/postgres/mini_postgres.h@96
PS9, Line 96: const std::string host_;
nit: mind adding a doc string for this field as well?
http://gerrit.cloudera.org:8080/#/c/15629/9/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/15629/9/src/kudu/postgres/mini_postgres.cc@93
PS9, Line 93: Kill
nit: maybe KillAndWait(SIGINT) is a better fit here?
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 Apr 2020 13:40:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 7:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15629/6/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/15629/6/src/kudu/postgres/mini_postgres.cc@146
PS6, Line 146: WaitForReady()
> nit: maybe rename this to WaitForReady() or something?
Done
http://gerrit.cloudera.org:8080/#/c/15629/6/src/kudu/postgres/mini_postgres.cc@147
PS6, Line 147:
> nit: can use FromSeconds()
Done
http://gerrit.cloudera.org:8080/#/c/15629/6/src/kudu/postgres/mini_postgres.cc@162
PS6, Line 162:
: return Status::TimedOut(s.ToString());
: }
> nit: If you do go down this route, could you call it 'deadline' instead of
Done
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 02 Apr 2020 09:23:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15629
to look at the new patch set (#10).
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
[postgres] Deflake MiniPostgres tests
Tests depending on MiniPostgres were flaky as in some cases Postgres
takes longer than expected to start. This commit introduces checking if
psql can connect to MiniPostgres as part of start-up and retries every
100ms for 5 seconds.
Ran MiniRangerTest 1000 times on dist-test with no failures with this
fix.
Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
---
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
2 files changed, 28 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/15629/10
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15629/6/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/15629/6/src/kudu/postgres/mini_postgres.cc@162
PS6, Line 162: if ((MonoTime::Now() - start) > kTimeout) {
: break;
: }
> nit: would be simpler as
nit: If you do go down this route, could you call it 'deadline' instead of 'timeout'?
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 22:27:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.h@88
PS4, Line 88: s connection t
> nit: can you add a comment for this method?
Done
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.cc@92
PS4, Line 92: // TODO(abukor): implement retry with a different port if it can't bind
: WARN_NOT_OK(process_->Kill(SIGINT), "failed to send SIGINT to Postgres");
: return wait;
: }
:
> RETURN_NOT_OK_PREPEND?
hm actually this is returning WaitForTcpBind()'s return value, but it's also important to kill the process if it fails, so I don't think that's applicable here.
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.cc@149
PS4, Line 149: MonoTime start = MonoTime::Now();
: while (true) {
: Subprocess psql({
: JoinPathSegments(bin_dir_, "postgres/pg_isready"),
: "-p", SimpleItoa(port_),
: "-h", host_,
: });
: RETURN_NOT_OK(psql.Start());
: s = psql.WaitAndCheckExitCode();
: if (s.ok()) {
: return s;
: }
:
: if ((MonoTime::Now() - start) > kTimeout) {
: break;
: }
: SleepFor(MonoDelta::FromMilliseconds(100));
: }
> nit: why bother with exponential backoff at all? We don't expect much load
Done
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.cc@168
PS4, Line 168: return s;
> Could you preserve the first (or last) result of WaitAndCheckExitCode(), an
Done
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 21:12:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 9:
> Patch Set 9: Code-Review+1
>
> (2 comments)
neither of these are part of this change, maybe in a follow-up commit?
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 Apr 2020 13:50:37 +0000
Gerrit-HasComments: No
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 14:22:25 +0000
Gerrit-HasComments: No
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15629
to look at the new patch set (#7).
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
[postgres] Deflake MiniPostgres tests
Tests depending on MiniPostgres were flaky as in some cases Postgres
takes longer than expected to start. This commit introduces checking if
psql can connect to MiniPostgres as part of start-up and retries the
connection up to 5 times with exponential back-off starting from 100ms.
Ran MiniRangerTest 1000 times on dist-test with no failures with this
fix.
Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
---
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
2 files changed, 28 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/15629/7
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15629/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15629/9//COMMIT_MSG@11
PS9, Line 11: retries every
: 100ms for 5 seconds.
> nit: update the commit msg?
Done
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 Apr 2020 17:12:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 Apr 2020 21:24:13 +0000
Gerrit-HasComments: No
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/15629/4/src/kudu/postgres/mini_postgres.cc@168
PS4, Line 168: return Status::RemoteError("Couldn't connect to Postgres");
Could you preserve the first (or last) result of WaitAndCheckExitCode(), and return it here, assuming it's worth something?
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 Apr 2020 19:26:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [postgres] Deflake MiniPostgres tests
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15629 )
Change subject: [postgres] Deflake MiniPostgres tests
......................................................................
Patch Set 9:
> > Patch Set 9: Code-Review+1
> >
> > (2 comments)
>
> neither of these are part of this change, maybe in a follow-up
> commit?
SGTM
--
To view, visit http://gerrit.cloudera.org:8080/15629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5106b3e2aeb9dabad9a7ee0b17059c1df2042bb4
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 Apr 2020 14:25:23 +0000
Gerrit-HasComments: No