Attila Bukor has posted comments on this change. ( )

Change subject: KUDU-3079 Add MiniRanger

Patch Set 46:

Commit Message:

> I don't see configurations for initializing/running Ranger Kudu plugin in t
PS44, Line 20: Python
> nit: python "2to3" program.
PS44, Line 20: but it required
> Can you list what are these manual changes?
PS44, Line 33:
> nit: REST
File build-support/
PS44, Line 97: # Tests that use the external minicluste
> nit: add a comment?
File build-support/
PS44, Line 178:   # co
> nit: indent?
File src/kudu/ranger/mini_ranger.h:
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 and DELETE and 
ALTER to johnsmith and janesmith to, you need to create one policy with 
two policy items.
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.
File src/kudu/ranger/
PS40, Line 60: LOG(INFO) << "Stopping Ranger...";
> nit: should we be consistent to log 'Starting Ranger' at Start() as well?
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_?
File src/kudu/ranger/mini_ranger_configs.h:
PS44, Line 64: admin port/RPC (REST
> It is not clear which port 'external ranger port' maps to in the doc you li
PS44, Line 92:
> nit: service
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.
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.
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
PS44, Line 313: </configuration>
              : )";
              : // Gets the contents of the file used by the
              : // script.
              : std::string GetRangerInstallProperties(std::string bin_dir, 
uint16_t pg_port) {
              :   return strings::Substitute(kInstallProperties, bin_dir, 
              : }
              : // 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, 
              : }
              : // 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, 
              :                              shutdown_port);
              : }
> nit: would you mind briefly documenting what is each method for? Especially

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15ab1eb8abe71c074c26b286073442882e101bc6
Gerrit-Change-Number: 15483
Gerrit-PatchSet: 46
Gerrit-Owner: Attila Bukor <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Andrew Wong <>
Gerrit-Reviewer: Attila Bukor <>
Gerrit-Reviewer: Grant Henke <>
Gerrit-Reviewer: Hao Hao <>
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