Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init ...................................................................... Patch Set 13: (26 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@225 PS10, Line 225: port > part Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.h@392 PS10, Line 392: postgres::MiniPostgres* postgres() const { > Here and below: to make this const-correct, either add add 'const' to the r this class is full of methods with similar "constness" should those be changed as well? http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.cc@23 PS10, Line 23: #include <algorithm> : #include <cstdint> > Why is this change? iwyu on my mac messes these up, restoring it to original shape 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@72 PS10, Line 72: std::string > Could be 'const std::string&' to avoid copying Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/postgres/mini_postgres.h@86 PS10, Line 86: IsRunning > It seems Process::IsStarted() might return 'true' if it's already exited. The goal is to be able to determine if the process was already started up, so we don't try to start it once more. Postgres fails to start up if it was already running or wasn't shut down properly. http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h File src/kudu/ranger-kms/mini_ranger_kms.h: http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@21 PS10, Line 21: > nit: remove the extra line Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@49 PS10, Line 49: explicit > nit: don't need explicit here Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@52 PS10, Line 52: > nit: wrong indent Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@62 PS10, Line 62: > nit: wrong indent Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@86 PS10, Line 86: : > nit: too many empty extra lines Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@114 PS10, Line 114: > nit: wrong indent Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@128 PS10, Line 128: > nit: wrong indent Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@138 PS10, Line 138: const std::string host_; > nit: maybe, add a short doc blurb for 'host_' as well? Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc File src/kudu/ranger-kms/mini_ranger_kms.cc: http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@95 PS10, Line 95: > nit: wrong indent Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@106 PS10, Line 106: > nit for here and below: misaligned indent Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@154 PS10, Line 154: RETURN_NOT_OK( : env_util::CopyFile(env_, : JoinPathSegments(kms_home, "kms-site.xml"), : JoinPathSegments(web_app_dir, : "WEB-INF/classes/conf/kms-site.xml"), : WritableFileOptions())); : : RETURN_NOT_OK( : env_util::CopyFile(env_, : JoinPathSegments(kms_home, "dbks-site.xml"), : JoinPathSegments(web_app_dir, : "WEB-INF/classes/conf/dbks-site.xml"), : WritableFileOptions())); : : RETURN_NOT_OK( : env_util::CopyFile(env_, : JoinPathSegments(kms_home, "log4j.properties"), : JoinPathSegments(web_app_dir, : "WEB-INF/classes/conf/log4j.properties"), : WritableFileOptions())); : RETURN_NOT_OK( : env_util::CopyFile(env_, : JoinPathSegments(kms_home, "ranger-kms-policymgr-ssl.xml"), : JoinPathSegments(web_app_dir, : "WEB-INF/classes/conf/ranger-kms-policymgr-ssl.xml"), : WritableFileOptions())); : : RETURN_NOT_OK( : env_util::CopyFile(env_, : JoinPathSegments(kms_home, "ranger-kms-security.xml"), : JoinPathSegments(web_app_dir, : "WEB-INF/classes/conf/ranger-kms-security.xml"), : WritableFileOptions())); : RETURN_NOT_OK( : env_util::CopyFile(env_, : JoinPathSegments(kms_home, "ranger-kms-site.xml"), : JoinPathSegments(web_app_dir,"WEB-INF/classes/conf/ranger-kms-site.xml"), : WritableFileOptions())); : : RETURN_NOT_OK( : env_util::CopyFile(env_, : JoinPathSegments(kms_home, "install.properties"), : JoinPathSegments(web_app_dir, "WEB-INF/classes/conf/install.properties"), : WritableFileOptions())); > Could rewrite this using 'for ()' cycle, defining an array of source file n Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@201 PS10, Line 201: > nit: wrong indent Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@210 PS10, Line 210: > nit: wrong indent Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@298 PS10, Line 298: > nit: wrong indent Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@333 PS10, Line 333: curl.FetchURL > What if FetchURL returns non-OK status? Should this be wrapped into RETURN Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h File src/kudu/ranger-kms/mini_ranger_kms_configs.h: http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h@42 PS10, Line 42: const char* > nit: could be 'const char* const' Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h@72 PS10, Line 72: kms_home > I'm not sure I see '$4' placeholder in kRangerKMSInstallPropertiesTemplate. Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h@79 PS10, Line 79: const char* > nit for here and below: could be 'constexpr const char* const' ? Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h@144 PS10, Line 144: const char * > nit: could be 'constexpr const char* const' ? Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger/mini_ranger.cc File src/kudu/ranger/mini_ranger.cc: http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger/mini_ranger.cc@21 PS10, Line 21: > nit: remove this extra empty line? Done http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/util/env_util.cc File src/kudu/util/env_util.cc: http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/util/env_util.cc@21 PS10, Line 21: #include <cerrno> > Why is this change? iwyu, restoring original -- 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: 13 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 11:42:21 +0000 Gerrit-HasComments: Yes
