Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: KUDU-2972 Add Ranger client ...................................................................... Patch Set 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client-test.cc File src/kudu/ranger/ranger_client-test.cc: http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client-test.cc@161 PS8, Line 161: ASSERT_TRUE(find(columns.begin(), columns.end(), "col1") != columns.end()); Use ContainsKey() from gutil/map-util.h Same elsewhere. http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.h@46 PS8, Line 46: // Authorizes an action on the table. Returns OK if 'user_name' is authorized : // to perform 'action' on 'table_name', NotAuthorized otherwise. Should also mention that this returns an error if the table name doesn't conform to Ranger's naming requirements; same with the other methods. Probably worth adding test cases for this too. http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.h@70 PS8, Line 70: std::shared_ptr<kudu::subprocess::SubprocessServer> server_; Could you add a TODO to make this a unique_ptr or a member variable when we no longer rely on the MockSubprocess test? http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc@141 PS8, Line 141: Status RangerClient::AuthorizeAction(const string& user_name, I'm fine doing this in another patch, but for these bulk requests, we ought to consider splitting up the Ranger requests across multiple messages in case they're larger than the expected max subprocess request size. Maybe add a TODO? http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc@153 PS8, Line 153: for (auto table : *table_names) { > warning: loop variable is copied but only used as const reference; consider Should use const auto& here http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc@154 PS8, Line 154: push_back nit: prefer emplace_back() instead of push_back(), here and elsewhere. http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc@158 PS8, Line 158: RETURN_NOT_OK(ParseRangerTableIdentifier(table, &db, &tbl)); This means that if there are any tables at all that don't conform to the Ranger requirements, we won't be able to list tables at all, right? Is that expected/desired vs just not authorizing non-conformant tables? If it is, could you update the comments in header to indicate that's the case? http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc@171 PS8, Line 171: for (auto i = 0; i < orig_table_names.size(); i++) { We should probably validate that responses_size() == orig_table_names.size(). Maybe CHECK/DCHECK/IllegalState() or somesuch? -- 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: 8 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, 24 Feb 2020 03:34:17 +0000 Gerrit-HasComments: Yes
