Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: [WIP] Ranger client ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/15206/3/src/kudu/ranger/ranger.proto File src/kudu/ranger/ranger.proto: http://gerrit.cloudera.org:8080/#/c/15206/3/src/kudu/ranger/ranger.proto@24 PS3, Line 24: SELE > To match with Ranger server side definition, we should use SELECT instead o Done http://gerrit.cloudera.org:8080/#/c/15206/3/src/kudu/ranger/ranger_action.cc File src/kudu/ranger/ranger_action.cc: http://gerrit.cloudera.org:8080/#/c/15206/3/src/kudu/ranger/ranger_action.cc@45 PS3, Line 45: case Action::METADATA: : return ActionPB::METADATA; > Throw exception for unknown actions? According to Google C++ style guide[1] we're not using exceptions. I could make it return a Status instead and return the action in an output parameter. I'm not sure it's worth it when using scoped enums though. What do you think? [1] https://google.github.io/styleguide/cppguide.html#Exceptions 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; > These three methods kind of overlap with each other, they are all for autho They overlap somewhat, but I think it's worth keeping it separate for simpler usage in its client (ranger_authz_provider). As for returning a list of booleans, we would need to convert the unordered_set to an ordered structure (e.g. vector) as the authz_provider API uses unordered_set. I think it would be wasteful to convert the unordered set to vector and back, especially as the communication between the ranger client and the subprocess is on the pipe and we don't need to save bandwidth on the wire. -- 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: 4 Gerrit-Owner: Attila Bukor <[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: Thu, 13 Feb 2020 14:36:21 +0000 Gerrit-HasComments: Yes
