Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: [WIP] Ranger client ...................................................................... Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/ranger/ranger.proto File src/kudu/ranger/ranger.proto: PS5: Seems like this conflicts with https://gerrit.cloudera.org/c/15074/4/src/kudu/ranger/ranger.proto, which makes me think we should have a separate patch that introduces the contents of ranger.proto that these two patches can then rebase onto once the interface is agreed upon. http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/ranger/ranger_client.h@65 PS5, Line 65: Status SendRequest(AuthzRequestPB* req, AuthzResponsePB* resp) WARN_UNUSED_RESULT; : : static Status AddTable(AuthzRequestPB* req, const std::string& table_name) WARN_UNUSED_RESULT; : : static void AddTable(std::unordered_set<std::string>* table_names, const TablePB& table); nit: add docs. http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/ranger/ranger_client.h@71 PS5, Line 71: std::shared_ptr<kudu::subprocess::SubprocessServer> server_; Is shared_ptr needed here? Did you consider using unique_ptr and having the RangerClient wholly own the subprocess? Or better yet have this be a stack-allocated member? We don't expect anything else to use it, right? http://gerrit.cloudera.org:8080/#/c/15206/3/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/15206/3/src/kudu/ranger/ranger_client.h@44 PS3, Line 44: // Authorizes listing tables. If there is at least one table that user is : // authorized to access metadata of, it returns OK and sets 'table_names' to : // the tables the user is authorized to list. Otherwise it returns : // NotAuthorized and it doesn't modify 'table_names'. : Status AuthorizeList(const std::string& user_name, : std::unordered_set<std::string>* table_names) WARN_UNUSED_RESULT; : : // 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 Action& action, : const std::string& table_name) WARN_UNUSED_RESULT; : : // Authorizes a scan on a table. Returns OK if 'user_name' is authorized to : // scan the whole table or at least one of the specified columns, : // NotAuthorized otherwise. If the user isn't authorized to scan the whole : // table, 'column_names' is changed to contain only the columns the user is : // authorized to scan. : Status AuthorizeScan(const std::string& user_name, const std::string& table_name, : std::unordered_set<std::string>* column_names) WARN_UNUSED_RESULT; > What is the reasoning to have duplicate code here and just to keep ranger_a FWIW I agree with Hao. I was kind of surprised to see so much business logic in what I expected to be a very simple, generic wrapper for the Java Ranger client. OTOH, should probably form some consensus on the desired interface we want in the Java Ranger client here https://gerrit.cloudera.org/c/15074/ http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/ranger/ranger_client.cc@60 PS5, Line 60: bool RangerClient::IsEnabled() { : return !FLAGS_ranger_policy_server.empty(); : } We probably don't need to plumb this all the way down here. Maybe just leave IsEnabled() to be implemented by the RangerAuthzProvider, and have the RangerAuthzProvider pass the Ranger policy server address to the RangerClient's constructor? It doesn't make much sense to construct a RangerClient at all if this is false. http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/subprocess/server.h File src/kudu/subprocess/server.h: http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/subprocess/server.h@86 PS5, Line 86: virtual ~SubprocessServer(); : : // Initialize the server, starting the subprocess and worker threads. : virtual Status Init() WARN_UNUSED_RESULT; : : // Synchronously send a request to the subprocess and populate 'resp' with : // contents returned from the subprocess, or return an error if anything : // failed or timed out along the way. : virtual Status Execute(SubprocessRequestPB* req, SubprocessResponsePB* resp) WARN_UNUSED_RESULT; Why these changes? http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/subprocess/server.cc File src/kudu/subprocess/server.cc: http://gerrit.cloudera.org:8080/#/c/15206/5/src/kudu/subprocess/server.cc@134 PS5, Line 134: || !process_->IsStarted()) { When do we shut down before calling Init()? -- 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: 5 Gerrit-Owner: Attila Bukor <[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: Mon, 17 Feb 2020 03:52:42 +0000 Gerrit-HasComments: Yes
