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 <abu...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <abu...@apache.org>
Gerrit-Reviewer: Grant Henke <granthe...@apache.org>
Gerrit-Reviewer: Hao Hao <hao....@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 24 Mar 2020 09:36:34 +0000
Gerrit-HasComments: Yes

Reply via email to