Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )
Change subject: KUDU-3079 Add MiniPostgres ...................................................................... Patch Set 21: (14 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? 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. 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. 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> (obviously not a big deal, but that's the style I think you're aiming for) 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]. 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: ############################## > did you also mean moving it to src/postgres? added tests that add users and I think you should include a full lifecycle test with persistence (i.e. start, do something, restart, verify that something was still done). 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: : > Yeah at the time I thought this was an issue with everything, but as it tur >From the execvp manpage: The const char *arg and subsequent ellipses in the execl(), execlp(), and execle() functions can be thought of as arg0, arg1, ..., argn. Together they describe a list of one or more pointers to null-terminated strings that represent the argument list available to the exe? cuted program. The first argument, by convention, should point to the filename associated with the file being executed. The list of arguments must be terminated by a null pointer, and, since these are variadic functions, this pointer must be cast (char *) NULL. The execv(), execvp(), and execvpe() functions provide an array of pointers to null-termi? nated strings that represent the argument list available to the new program. The first argument, by convention, should point to the filename associated with the file being exe? cuted. The array of pointers must be terminated by a null pointer. Seems like our behavior is wrong and we should ensure the first argument really is the full path to the file being executed. I'm fine with you tackling this in a follow-up though. http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@101 PS15, Line 101: > hm good point, I don't think we need the full $PATH here though, so it shou Bear in mind that if PATH is constrained to just <bin_dir>/postgres, if Postgres does any PATH-based execution of its own (i.e. another subprocess), it probably won't find what it's looking for. Do you expect that to happen? You could evaluate the current process' $PATH, add <bin_dir>/postgres to it, and use the result here. http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@107 PS15, Line 107: : > seems UNIQUE_LOOPBACK is not supported on MacOS, at least according to the By "this" I meant "make postgres support binding to 0". Anyway, just because macOS doesn't support something that Linux doesn't mean we shouldn't do it. If you want to tackle this post-merge that's fine, but I'll be surprised if the randomized port approach doesn't cause at least some test flakiness when considering how many tests we run. 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@558 PS21, Line 558: Status status = DoWait(&wait_status, BLOCKING); : if (!status.ok()) { : return status; : } Could just RETURN_NOT_OK? http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@567 PS21, Line 567: RemoteError RuntimeError is more appropriate here. 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. 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? Just curious. http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh@1035 PS21, Line 1035: # simplify build Nit: terminate with a period. -- 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 22:04:58 +0000 Gerrit-HasComments: Yes
