Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15483 )
Change subject: KUDU-3079 Add MiniRanger ...................................................................... Patch Set 46: (16 comments) 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 t Done http://gerrit.cloudera.org:8080/#/c/15483/44//COMMIT_MSG@20 PS44, Line 20: Python > nit: python "2to3" program. Done http://gerrit.cloudera.org:8080/#/c/15483/44//COMMIT_MSG@20 PS44, Line 20: but it required > Can you list what are these manual changes? Done http://gerrit.cloudera.org:8080/#/c/15483/44//COMMIT_MSG@33 PS44, Line 33: > nit: REST Done 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: # Tests that use the external minicluste > nit: add a comment? Done 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: # co > nit: indent? Done 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 separ You can't create multiple policies with identical sets of resources, so if you want to give SELECT and INSERT to johndoe and janedoe on foo.bar and DELETE and ALTER to johnsmith and janesmith to foo.bar, you need to create one policy with two policy items. 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 it isn't only needed by fresh install, fresh install simply indicates if admin home existed before calling InitRanger. admin_home is the home directory for Ranger admin. 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? Done http://gerrit.cloudera.org:8080/#/c/15483/40/src/kudu/ranger/mini_ranger.cc@87 PS40, Line 87: ranger_port_ is the RPC port (REST API) that the Ranger subprocess and : // EasyCurl can talk to > nit: can you elaborate a bit more on which RPC is for with ranger_port_? Done 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: admin port/RPC (REST > It is not clear which port 'external ranger port' maps to in the doc you li Done http://gerrit.cloudera.org:8080/#/c/15483/44/src/kudu/ranger/mini_ranger_configs.h@92 PS44, Line 92: > nit: service Done http://gerrit.cloudera.org:8080/#/c/15483/44/src/kudu/ranger/mini_ranger_configs.h@101 PS44, Line 101: ptio > Also will this be configurable on a secure env? I'm not sure what exactly will be needed for a secure env so din't want to make too many assumptions, we can always change this. http://gerrit.cloudera.org:8080/#/c/15483/44/src/kudu/ranger/mini_ranger_configs.h@105 PS44, Line 105: >NONE</value> > Should this be configurable when Kerberos is enabled? probably, but didn't want to make too many assumptions as mentioned above. http://gerrit.cloudera.org:8080/#/c/15483/44/src/kudu/ranger/mini_ranger_configs.h@247 PS44, Line 247: g4j.rootL > nit: what is xa_logger for? this is the main logger I think, this is unmodified, except for the log level above. I added a comment http://gerrit.cloudera.org:8080/#/c/15483/44/src/kudu/ranger/mini_ranger_configs.h@313 PS44, Line 313: </configuration> : )"; : : // Gets the contents of the install.properties file used by the db_setup.py : // script. : std::string GetRangerInstallProperties(std::string bin_dir, uint16_t pg_port) { : return strings::Substitute(kInstallProperties, bin_dir, pg_port); : } : : // Gets the contents of the ranger-admin-site.xml config that has most of the : // configuration needed to start Ranger. : std::string GetRangerAdminSiteXml(uint16_t port, uint16_t pg_port) { : return strings::Substitute(kRangerAdminSiteTemplate, pg_port, port); : } : : // Gets the ranger-admin-default-site.xml that has some additional configuration : // needed to start Ranger. It's unclear why this has to be a separate file. : std::string GetRangerAdminDefaultSiteXml(std::string pg_driver, : uint16_t shutdown_port) { : return strings::Substitute(kRangerAdminDefaultSiteTemplate, pg_driver, : shutdown_port); : } : : > nit: would you mind briefly documenting what is each method for? Especially Done -- 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: 46 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: Tue, 24 Mar 2020 09:36:34 +0000 Gerrit-HasComments: Yes
