Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15483 )
Change subject: KUDU-3079 Add MiniRanger ...................................................................... Patch Set 44: (16 comments) Thanks a lot Attila for working on the MiniRanger patch! http://gerrit.cloudera.org:8080/#/c/15483/44//COMMIT_MSG Commit Message: PS44: I don't see configurations for initializing/running Ranger Kudu plugin in this commit. If we are going to do that in a follow up patch, we probably want to note that in the commit msg? http://gerrit.cloudera.org:8080/#/c/15483/44//COMMIT_MSG@20 PS44, Line 20: "2to3" nit: python "2to3" program. http://gerrit.cloudera.org:8080/#/c/15483/44//COMMIT_MSG@20 PS44, Line 20: some manual changes Can you list what are these manual changes? http://gerrit.cloudera.org:8080/#/c/15483/44//COMMIT_MSG@33 PS44, Line 33: RESY nit: REST http://gerrit.cloudera.org:8080/#/c/15483/44/build-support/dist_test.py File build-support/dist_test.py: http://gerrit.cloudera.org:8080/#/c/15483/44/build-support/dist_test.py@97 PS44, Line 97: "thirdparty/installed/common/opt/ranger" nit: add a comment? http://gerrit.cloudera.org:8080/#/c/15483/44/build-support/run_dist_test.py File build-support/run_dist_test.py: http://gerrit.cloudera.org:8080/#/c/15483/44/build-support/run_dist_test.py@178 PS44, Line 178: # Crea nit: indent? http://gerrit.cloudera.org:8080/#/c/15483/40/src/kudu/ranger/mini_ranger.h File src/kudu/ranger/mini_ranger.h: http://gerrit.cloudera.org:8080/#/c/15483/40/src/kudu/ranger/mini_ranger.h@50 PS40, Line 50: UserList Can you elaborate why do we need a list here, why not have user to be separated from AuthorizationPolicy? And then associate user with AuthorizationPolicy? http://gerrit.cloudera.org:8080/#/c/15483/40/src/kudu/ranger/mini_ranger.h@91 PS40, Line 91: admin_home nit: can you elaborate what 'admin_home' is for and why only fresh install need it? Also what does the initialization do other than create admin_home. http://gerrit.cloudera.org:8080/#/c/15483/40/src/kudu/ranger/mini_ranger.cc File src/kudu/ranger/mini_ranger.cc: http://gerrit.cloudera.org:8080/#/c/15483/40/src/kudu/ranger/mini_ranger.cc@60 PS40, Line 60: LOG(INFO) << "Stopping Ranger..."; nit: should we be consistent to log 'Starting Ranger' at Start() as well? http://gerrit.cloudera.org:8080/#/c/15483/40/src/kudu/ranger/mini_ranger.cc@87 PS40, Line 87: ranger_port_ is the RPC port. : // - ranger_admin_port is the port for the admin REST API nit: can you elaborate a bit more on which RPC is for with ranger_port_? http://gerrit.cloudera.org:8080/#/c/15483/44/src/kudu/ranger/mini_ranger_configs.h File src/kudu/ranger/mini_ranger_configs.h: http://gerrit.cloudera.org:8080/#/c/15483/44/src/kudu/ranger/mini_ranger_configs.h@64 PS44, Line 64: external ranger port It is not clear which port 'external ranger port' maps to in the doc you linked above. http://gerrit.cloudera.org:8080/#/c/15483/44/src/kudu/ranger/mini_ranger_configs.h@92 PS44, Line 92: Serice nit: service http://gerrit.cloudera.org:8080/#/c/15483/44/src/kudu/ranger/mini_ranger_configs.h@101 PS44, Line 101: true Also will this be configurable on a secure env? http://gerrit.cloudera.org:8080/#/c/15483/44/src/kudu/ranger/mini_ranger_configs.h@105 PS44, Line 105: ranger.authentication.method Should this be configurable when Kerberos is enabled? http://gerrit.cloudera.org:8080/#/c/15483/44/src/kudu/ranger/mini_ranger_configs.h@247 PS44, Line 247: xa_logger nit: what is xa_logger for? http://gerrit.cloudera.org:8080/#/c/15483/44/src/kudu/ranger/mini_ranger_configs.h@313 PS44, Line 313: std::string GetRangerInstallProperties(std::string bin_dir, uint16_t pg_port) { : return strings::Substitute(kInstallProperties, bin_dir, pg_port); : } : : std::string GetRangerAdminSiteXml(uint16_t port, uint16_t pg_port, : uint16_t admin_port) { : return strings::Substitute(kRangerAdminSiteTemplate, pg_port, : port, admin_port); : } : : std::string GetRangerAdminDefaultSiteXml(std::string pg_driver, : uint16_t shutdown_port) { : return strings::Substitute(kRangerAdminDefaultSiteTemplate, pg_driver, : shutdown_port); : } : : std::string GetRangerLog4jProperties(std::string log_level) { : return strings::Substitute(kLog4jPropertiesTemplate, log_level); : } : : std::string GetRangerCoreSiteXml(bool secure) { : return strings::Substitute(kCoreSiteTemplate, secure ? "kerberos" : "simple"); : } : nit: would you mind briefly documenting what is each method for? Especially it is not clear from the name what is the difference between GetRangerAdminSiteXml and GetRangerAdminDefaultSiteXml. -- To view, visit http://gerrit.cloudera.org:8080/15483 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I15ab1eb8abe71c074c26b286073442882e101bc6 Gerrit-Change-Number: 15483 Gerrit-PatchSet: 44 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: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 23 Mar 2020 22:59:05 +0000 Gerrit-HasComments: Yes
