Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: KUDU-2972 Add Ranger client ...................................................................... Patch Set 21: (9 comments) http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger.proto File src/kudu/ranger/ranger.proto: http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger.proto@31 PS20, Line 31: check > check out? Done http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client-test.cc File src/kudu/ranger/ranger_client-test.cc: http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client-test.cc@112 PS16, Line 112: action.column_name = req.has_column() ? req.column() : ""; > Oh oops, thought it returned a Status. This is fine for a test. Done 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: }; : : typedef subprocess::SubprocessProxy<RangerRequestListPB, RangerResponseListPB, : RangerSubprocessMetrics> RangerSubprocess; : > Is this needed if we have AuthorizeActions()? Not necessarily needed but it's simpler to use than AuthorizeActions() and the code is less complex too so I'd rather leave it in. http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.h@69 PS20, Line 69: > nit: "multiple table-level actions" Done http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.h@74 PS20, Line 74: // columns the user is authorized to access and returns OK, NotAuthorized > nit: per the GSG, output parameters should be last. Done 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: kudu::MetricUnit::kMilliseconds, : "Duration of time in ms spent in the Ranger subprocess' outbound response queue", : kudu::MetricLevel::kInfo, : 60000LU, 1); : METRIC_DEFINE_histogram(server, ranger_subprocess_execution_time_ms, : "Ranger subprocess execution time (ms)", : kudu::MetricUnit::kMilliseconds, : "Duration of time in ms spent executing the Ranger subprocess request, excluding " : "time spent spent in the subprocess queues", : kudu::MetricLevel::kInfo, : 60000LU, 1); : : namespace kudu { : n > Depending on the order of what lands, may want to rebase on this and typede rebased it, using yours now. Had to change the proxy up a bit (heap allocated server so it can be mocked and a ForTests method to replace it) http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.cc@112 PS20, Line 112: HISTINIT(execution_time_ms, ranger_subpr > nit: flip the order of the arguments so the error message is more correct ( Done http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.cc@276 PS20, Line 276: : RangerRequestListPB req_list; : RangerResponseListPB resp_list; : req_list.set_user(user_name); : : for (const auto& action : *actions) { : > Will the JARs always be located in the binary directory? I was under the im Done 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: // : // Normally the process_ should be started before we reach Shutdown() and the : // threads below should be running too, except in mock servers because we : // don't init there. Shutdown() is still called in this case from the : // destructor though so these checks are necessary. : 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(); : } : i > nit: could you mention in a comment that these are all expected to always b Done -- 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: 21 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: Wed, 04 Mar 2020 11:16:53 +0000 Gerrit-HasComments: Yes
