Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )
Change subject: KUDU-3079 Add MiniPostgres ...................................................................... Patch Set 21: (20 comments) http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres-test.cc File src/kudu/postgres/mini_postgres-test.cc: http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres-test.cc@28 PS21, Line 28: class PostgresTest : public KuduTest { > How long does the individual test start/stop sequence take? It's surprisingly fast, these tests all finish within 1.5 seconds on my Mac. http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres-test.cc@43 PS21, Line 43: Status createdb = postgres_.CreateDb("testdb", "nonexistentuser"); : ASSERT_TRUE(createdb.IsRemoteError()); > Can combine. Done http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.h File src/kudu/postgres/mini_postgres.h: http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.h@82 PS21, Line 82: env->GetExecutablePath(&exe); > nit: CHECK_OK here or log if this fails? Done http://gerrit.cloudera.org:8080/#/c/15374/20/src/kudu/postgres/mini_postgres.h File src/kudu/postgres/mini_postgres.h: http://gerrit.cloudera.org:8080/#/c/15374/20/src/kudu/postgres/mini_postgres.h@53 PS20, Line 53: // bypasssed[1] > Nit: terminate with a period. Done http://gerrit.cloudera.org:8080/#/c/15374/20/src/kudu/postgres/mini_postgres.h@55 PS20, Line 55: // [1] https://www.postgresql.org/docs/12/role-attributes.html > Nit: this should be 1. <url> Done http://gerrit.cloudera.org:8080/#/c/15374/20/src/kudu/postgres/mini_postgres.h@60 PS20, Line 60: // owners.[1] > Nit: move the period to after [1]. Done http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc File src/kudu/postgres/mini_postgres.cc: http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@57 PS21, Line 57: this-> > nit: remove? Done http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@62 PS21, Line 62: unique_ptr > nit: this doesn't need to be heap-allocated Done http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@90 PS21, Line 90: "$0:$$PATH" > Do we need to evaluate this? Done http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@117 PS21, Line 117: Substitute("$0", port_) > nit: could we use IntToString() from gutil/strings/numbers.h instead of Sub Done http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@133 PS21, Line 133: const string& pg_root > nit: rather than allowing this to be any path/string, how about taking no a Done http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/CMakeLists.txt File src/kudu/ranger/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/CMakeLists.txt@35 PS15, Line 35: ############################## > I think you should include a full lifecycle test with persistence (i.e. sta do you think testing persistence with DDL (user creation) is enough or should that do something be something DML? http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc File src/kudu/ranger/mini_postgres.cc: http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@97 PS15, Line 97: : > From the execvp manpage: Done http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@107 PS15, Line 107: : > By "this" I meant "make postgres support binding to 0". I'm not sure how we can make it support binding to 0. Seems like an intentional choice, should we try to patch it in thirdparty? I'm not convinced the randomized port approach would cause flakiness as it's effectively the same as binding to 0. We're essentially binding to 0, then release the port and then immediately binding to that port again. The kernel should also guarantee it's not reassigned for 2 minutes unless the defaults were changed on the hosts we're running the tests on. Anyway, I agree we can tackle this in a follow-up commit if we see it leads to flakiness. http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@556 PS21, Line 556: Status Subprocess::WaitAndCheckExitCode() { > Should this be reusing GetExitStatus()? If not, why not? Done http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@558 PS21, Line 558: Status status = DoWait(&wait_status, BLOCKING); : if (!status.ok()) { : return status; : } > Could just RETURN_NOT_OK? Done http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@567 PS21, Line 567: RemoteError > RuntimeError is more appropriate here. Done http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@574 PS21, Line 574: : return Status::IllegalState(Substitute("Stopped with signal $0", : WSTOPSIG(wait_status))); > Should enforce that WIFSTOPPED is true before doing this. Done http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh@1034 PS21, Line 1034: # We don't need extra features like readline and zlib so so let's just > In what way does this simplify the build? Faster? Fewer system dependencies fewer system dependencies and presumably faster build, although I didn't benchmark and the build is fast enough. Should I remove and just use the defaults where we don't need to override? http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh@1035 PS21, Line 1035: # simplify build > Nit: terminate with a period. Done -- To view, visit http://gerrit.cloudera.org:8080/15374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc Gerrit-Change-Number: 15374 Gerrit-PatchSet: 21 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sun, 22 Mar 2020 23:39:39 +0000 Gerrit-HasComments: Yes
