Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init ...................................................................... Patch Set 15: (13 comments) http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.h@392 PS10, Line 392: postgres::MiniPostgres* postgres() const { > this class is full of methods with similar "constness" should those be chan OK, if you prefer this to be uniform in that regard, the const-correctness piece could be fixed in a separate changelist -- totally fine with me. http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/postgres/mini_postgres.h File src/kudu/postgres/mini_postgres.h: http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/postgres/mini_postgres.h@86 PS10, Line 86: IsRunning > The goal is to be able to determine if the process was already started up, Right: my point was that your new method called 'IsRunning()' returns 'true' even after calling 'KillAndWait(SIGTERM)' on the Postgres process. That's why I suggested to rename it into IsStarted(). IsRunning() has semantics of 'the process is running now', and that's not exactly captured by the Subprocess::IsStarted() method. http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.h File src/kudu/ranger-kms/mini_ranger_kms.h: http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.h@49 PS15, Line 49: std::shared_ptr<postgres::MiniPostgres> mini_pg, : std::shared_ptr<ranger::MiniRanger> mini_ranger) nit: misaligned indent http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.h@51 PS15, Line 51: nit: this supposed to be 2 + 4 spaces http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.h@61 PS15, Line 61: nit: this supposed to be 2 + 4 spaces Take a look at https://github.com/apache/kudu/blob/8902ca6911b348ec3737da0e5f670726642f9fb0/src/kudu/kserver/kserver.cc#L136-L141 for an example. http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.h@67 PS15, Line 67: nit: this supposed to be 2 spaces http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.h@135 PS15, Line 135: RangeKMS nit: RangerKMS http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.cc File src/kudu/ranger-kms/mini_ranger_kms.cc: http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.cc@148 PS15, Line 148: vector<string> files_to_copy = { nit: could be 'static const' http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.cc@149 PS15, Line 149: "dbks-site.xml", : "install.properties" : "kms-site.xml" : "log4j.properties", : "ranger-kms-policymgr-ssl.xml", : "ranger-kms-security.xml", nit: wrong indent http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.cc@157 PS15, Line 157: auto nit: could be 'const auto&' http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.cc@265 PS15, Line 265: port Do you want to update the 'port_' member at some point with the value of 'port'? http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger/mini_ranger.h File src/kudu/ranger/mini_ranger.h: http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger/mini_ranger.h@75 PS15, Line 75: explicit MiniRanger(std::string host, std::shared_ptr<postgres::MiniPostgres> mini_pg) : : MiniRanger(GetTestDataDirectory(), std::move(host), std::move(mini_pg)) {} nit: incorrect formatting For an example of correct one, please check out https://github.com/apache/kudu/blob/8902ca6911b348ec3737da0e5f670726642f9fb0/src/kudu/kserver/kserver.cc#L136-L141 http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger/mini_ranger.h@147 PS15, Line 147: nit: wrong indent -- To view, visit http://gerrit.cloudera.org:8080/18645 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f Gerrit-Change-Number: 18645 Gerrit-PatchSet: 15 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Fri, 22 Jul 2022 15:44:49 +0000 Gerrit-HasComments: Yes
