Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: KUDU-2972 Add Ranger client ...................................................................... Patch Set 20: (7 comments) http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.h@46 PS20, Line 46: : // Authorizes an action on the table. Returns OK if 'user_name' is authorized : // to perform 'action' on 'table_name', NotAuthorized otherwise. : Status AuthorizeAction(const std::string& user_name, const ActionPB& action, : const std::string& table_name) WARN_UNUSED_RESULT; Is this needed if we have AuthorizeActions()? http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.h@69 PS20, Line 69: a table-level action nit: "multiple table-level actions" http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.h@74 PS20, Line 74: std::unordered_set<ActionPB, ActionHash>* actions, nit: per the GSG, output parameters should be last. http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.cc@73 PS20, Line 73: : Status RangerClient::SendRequest(RangerRequestListPB* req, RangerResponseListPB* resp) { : SubprocessRequestPB sreq; : SubprocessResponsePB sresp; : : auto packed_req = new Any(); : packed_req->PackFrom(*req); : sreq.set_allocated_request(packed_req); : RETURN_NOT_OK(server_->Execute(&sreq, &sresp)); : : CHECK(sresp.response().UnpackTo(resp)); : : return Status::OK(); : } Depending on the order of what lands, may want to rebase on this and typedef some RangerSubprocess: https://gerrit.cloudera.org/c/15344/ Though we can always address that later too. http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.cc@112 PS20, Line 112: CHECK_EQ(resp_list.responses_size(), 1); nit: flip the order of the arguments so the error message is more correct (expected value first). Same elsewhere. http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.cc@276 PS20, Line 276: string RangerClient::GetJavaClasspath() { : Env* env = Env::Default(); : string exe; : CHECK_OK(env->GetExecutablePath(&exe)); : const string bin_dir = DirName(exe); : return Substitute("$0/kudu-ranger.jar:$1", bin_dir, FLAGS_ranger_config_path); : } Will the JARs always be located in the binary directory? I was under the impression that many deployments have their own separate directory for JARs (e.g. Kudu backup JARs in CDH are located in a JARs directory). If that's the case, would it make sense to have the JAR path be configurable? http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/subprocess/server.cc File src/kudu/subprocess/server.cc: http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/subprocess/server.cc@147 PS20, Line 147: if (process_->IsStarted()) { : WARN_NOT_OK(process_->KillAndWait(SIGTERM), "failed to stop subprocess"); : } : inbound_response_queue_.Shutdown(); : outbound_call_queue_.Shutdown(); : : // We should be able to clean up our threads; they'll see that we're closing, : // the pipe has been closed, or the queues have been shut down. : if (write_thread_) { : write_thread_->Join(); : } : if (read_thread_) { : read_thread_->Join(); : } : if (deadline_checker_) { : deadline_checker_->Join(); : } nit: could you mention in a comment that these are all expected to always be there, except for in the mock? -- To view, visit http://gerrit.cloudera.org:8080/15206 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4 Gerrit-Change-Number: 15206 Gerrit-PatchSet: 20 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 03 Mar 2020 23:00:27 +0000 Gerrit-HasComments: Yes
