Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )
Change subject: KUDU-3079 Add MiniPostgres ...................................................................... Patch Set 11: (7 comments) http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.h File src/kudu/ranger/mini_postgres.h: http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.h@35 PS9, Line 35: > warning: single-argument constructors must be marked explicit to avoid unin Done http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc File src/kudu/ranger/mini_postgres.cc: http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@102 PS6, Line 102: we need to get a random unused : // port and persist that to the config file > Different miniclusters should get different directories by virtue of using I was thinking about mini clusters that you create manually with "kudu test mini_cluster", I think those will use the same directory. Added a comment to GetRandomPort() about how this works. We have 2 minutes to start up Ranger after running GetRandomPort() which should be more than enough, unless someone lowers tcp_fin_timeout significantly which is discouraged, and even then it's unlikely to cause issues as the kernel would need to exhaust the ephemeral port range first. // Gets a random port from the ephemeral range by binding to port 0 and letting // the kernel choose an unused one from the ephemeral port range. The socket is // then immediately closed and it remains in TIME_WAIT for 2*tcp_fin_timeout (by // default 2*60=120 seconds). The kernel won't assign this port until it's in // this state but it can still be used by binding it explicitly. http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@146 PS6, Line 146: : Status MiniPostgres::CreateDb(const string& db, const string& owner) { : Subprocess createdb({ : JoinPathSegments(bin_dir_, "postgres/createdb"), : "-O", owner, db, : "-p", Substitute("$0", port_), : }); : RETURN_NOT_OK(createdb.Start()); : return createdb.Wait(); > initdb creates these, added a comment removed this part http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc File src/kudu/ranger/mini_postgres.cc: http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@45 PS9, Line 45: namespace kudu { > nit: add a comment for the method. Done http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@105 PS9, Line 105: TURN_NOT_OK(CreateConfigs(pg_root)); > nit: can you comment on why this is needed? Done http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@153 PS9, Line 153: db.Start()); > Is there a config for where the log will go? By default it logs to stderr, but it can be configured to log to file. stderr should be fine for us, what do you think? http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@157 PS9, Line 157: : Status MiniPostgres > Will these config affect the tests environment? I'm not sure what you mean. Do you think the min_wal_size should be set lower? -- 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: 11 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: Thu, 19 Mar 2020 21:02:12 +0000 Gerrit-HasComments: Yes
