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 10: (29 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 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 return type of remove the 'const' method specifier. https://isocpp.org/wiki/faq/const-correctness 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? 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 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. With that, does it make call this method 'IsStarted' as well or here the point is to check whether the process is currently running? 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 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 http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@52 PS10, Line 52: nit: wrong indent http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@62 PS10, Line 62: nit: wrong indent http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@78 PS10, Line 78: GetKeys nit: could be a constant method? 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 http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@114 PS10, Line 114: nit: wrong indent http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@128 PS10, Line 128: nit: wrong indent 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? 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 http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@104 PS10, Line 104: nit here and belwo: wrong indent 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 http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@137 PS10, Line 137: return Status::OK(); nit: could remove this and just call return at line 132 instead of wrapping WriteStringToFile() into RETURN_NOT_OK 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 names. http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@201 PS10, Line 201: nit: wrong indent http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@210 PS10, Line 210: nit: wrong indent http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@298 PS10, Line 298: nit: wrong indent 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_NOT_OK()? 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' 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. Also, it doesn't align with the comment for $4 at kRangerKMSInstallPropertiesTemplate 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' ? 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' ? Also, per code style guide, should stick the asterisk to the type, not to the variable name. 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? 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? -- 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: 10 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: Thu, 21 Jul 2022 19:50:53 +0000 Gerrit-HasComments: Yes
