Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )
Change subject: KUDU-3079 Add MiniPostgres ...................................................................... Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/15374/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15374/6//COMMIT_MSG@17 PS6, Line 17: PostgrSQL PostgreSQL http://gerrit.cloudera.org:8080/#/c/15374/6//COMMIT_MSG@28 PS6, Line 28: https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html extra line break? 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@53 PS6, Line 53: listener.Close(); Is it possible to drop this? Socket is going out of the scope upon return from the function, so Close() will be called automatically from the destructor. http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@99 PS6, Line 99: nit: it seems shift should be 4 spaces in this case? http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@102 PS6, Line 102: so generate a port now. : // TODO(awong): keep trying until we succeed. It seems this comment is outdated, no? The port number was generated above at line 94, right? http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@108 PS6, Line 108: SIGQUIT SIGINT? http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@110 PS6, Line 110: LOG(INFO) << "Postgres bound to " << port_; Is this still true when !wait.ok() ? http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@115 PS6, Line 115: RETURN_NOT_OK_PREPEND(process_->KillAndWait(SIGTERM), "failed to stop Postgres"); : return Status::OK(); nit: replace with return process_->KillAndWait(SIGTERM), "failed to stop Postgres"); ? http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@146 PS6, Line 146: dynamic_shared_memory_type = posix # the default is the first option : max_wal_size = 1GB : min_wal_size = 80MB : datestyle = 'iso, mdy' : lc_messages = 'en_US.UTF-8' # locale for system error message : lc_monetary = 'en_US.UTF-8' # locale for monetary formatting : lc_numeric = 'en_US.UTF-8' # locale for number formatting : lc_time = 'en_US.UTF-8' # locale for time formatting : default_text_search_config = 'pg_catalog.english' If these are non-default settings, could you add a comment w.r.t why the default setting was not good enough? -- 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: 6 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 04:51:46 +0000 Gerrit-HasComments: Yes
