Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15483 )
Change subject: KUDU-3079 Add MiniRanger ...................................................................... Patch Set 24: (13 comments) http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/CMakeLists.txt File src/kudu/ranger/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/CMakeLists.txt@53 PS22, Line 53: : execute_process(COMMAND ln -nsf : "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/ranger" : "${EXECUTABLE_OUTPUT_PATH}/ranger-home") : execute_process(COMMAND ln -nsf : "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/hadoop" : "${EXECUTABLE_OUTPUT_PATH}/hadoop-home") : execute_process(COMMAND ln -nsf : "${JAVA_HOME}" : "${EXECUTABLE_OUTPUT_PATH}/java-home") > Can this be removed now that Postgres is in its own module? Done http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.h File src/kudu/ranger/mini_ranger.h: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.h@40 PS22, Line 40: > nit: maybe typedef this too as UserList so this is more self-documenting. Done http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.h@47 PS22, Line 47: // AuthorizationPolicy. Number of users and actions doesn't have to match, their : // cross-product is taken. : typedef std::pair<UserList, std::vector<ActionPB>> PolicyItem; : > nit: it isn't clear upon reading the comment whether the vectors here shoul Added a clarification. Your model is a bit off, each policy can have multiple databases/tables/columns and the users/actions product is not tied to a single one of them, but all of them in the same policy. http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc File src/kudu/ranger/mini_ranger.cc: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@61 PS22, Line 61: RETURN_NOT_OK(r > RETURN_NOT_OK Done http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@195 PS22, Line 195: "-Dservername=miniranger", : Substitute("-Dcatalina.base=$0", kEwsDir), : Substitute("-Dlogdir=$0", JoinPathSegments(kAdminHome, "logs")), : "-Dranger.audit.solr.bootstrap.enabled=false", : "-cp", classpath, "org.apache.ranger.server.tomcat.EmbeddedServer" : })); : ranger_process_->SetEnvVars({ : { "XAPOLICYMGR_DIR", kAdminHome }, : { "XAPOLICYMGR_EWS_DIR", kEwsDir }, : { "RANGER_JAAS_LIB_DIR", JoinPathSegments(kWebAppDir, "ranger_jaas") }, : { "RANGER_JAAS_CONF_DIR", JoinPathSegments(kConfDir, "ranger_jaas") }, : { "JAVA_HOME", java_home_ }, : { "RANGER_PID_DI > Probably don't need these comments anymore, but it's probably worth linking interestingly I can't find ranger-admin-services.sh in the repo, not sure where it's coming from. http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@234 PS22, Line 234: if (s.IsRemoteError()) { > You can remove these :) Done http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger.cc@256 PS22, Line 256: : : if (!policy.tables.empty()) { : EasyJson tables = resources.Set("table", EasyJson::kObject); : EasyJson table_values = tables.Set("values", EasyJson::kArray); : for (const string& table : policy.tables) { : table_values.PushBack(table); : } : } > nit: shift back a couple spaces Done http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger_configs.h File src/kudu/ranger/mini_ranger_configs.h: PS22: > Thanks for grokking and organizing these configs! Makes it significantly mo Done http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger_configs.h@63 PS22, Line 63: postgres port > nit: could you reword this? Matters for what? Done http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger_configs.h@78 PS22, Line 78: 0 > nit: reorder with $0? Done http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger_configs.h@139 PS22, Line 139: <property> : <name>ranger.service.shutdown.port</name> : <value>$1</value> : </property> > Now that we're just using SIGTERM, do we still need this? unfortunately yes, otherwise it would just bind to 6185 and cause collisions in tests http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/ranger/mini_ranger_configs.h@224 PS22, Line 224: jdbc:log4jdbc:mysql > postgresql? Does this actually matter? it doesn't matter, but it fails to start without it as it still tries to resolve ranger.jpa.audit.jdbc.url, so I just left it on default. http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/15483/22/src/kudu/tools/tool.proto@45 PS22, Line 45: // Whether or not Ranger should be enabled : optional bool enable_ranger = 9; : > Hrm, why don't we have an enable_sentry? Will we actually use this? not sure why Sentry doesn't have one. I did this so I can test the REST API using mini cluster instead of waiting in a test. Should I break it out? -- 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: 24 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 00:57:29 +0000 Gerrit-HasComments: Yes
