Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: KUDU-2972 Add Ranger client ...................................................................... Patch Set 24: (5 comments) http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/ranger/ranger_client.cc@118 PS23, Line 118: TODO(abukor): refactor to avoid code duplication : Status RangerClient::AuthorizeAction(const string& user_name, : const ActionPB& action, : const string& table_name) { : string db; : Slice tbl; : : auto s = ParseRangerTableIdentifier(table_name, &db, &tbl); : if (PREDICT_FALSE(!s.ok())) { : LOG(WARNING) << Substitute(kDenyNonRangerTableTemplate, table_name); : return Status::NotAuthorized(kUnauthorizedAction); : } : : RangerRequestListPB req_list; : RangerResponseListPB resp_list; : req_list.set_user(user_name); : : RangerRequestPB* req = req_list.add_requests(); : : req->set_action(action); : req->set_database(db); : req->set_table(tbl.ToString()); : : RETURN_NOT_OK(subprocess_.Execute(req_list, &resp_list)); : : CHECK_EQ(1, resp_list.responses_size()); : if (resp_list.responses().begin()->allowed()) { : return Status::OK(); : } > nit: Could this be: as discussed offline it seems there's a significant (~20%) perf impact between these methods, so for now I'm just adding a todo to refactor it to avoid code duplication [ RUN ] RangerClientTest.BenchmarkAuthorizeAction [ OK ] RangerClientTest.BenchmarkAuthorizeAction (8202 ms) [ RUN ] RangerClientTest.BenchmarkAuthorizeActions [ OK ] RangerClientTest.BenchmarkAuthorizeActions (9774 ms) TEST_F(RangerClientTest, BenchmarkAuthorizeAction) { Allow("jdoe", ActionPB::SELECT, "default", "foobar"); for (int i = 0; i < 1000000; ++i) { ASSERT_OK(client_.AuthorizeAction("jdoe", ActionPB::SELECT, "default.foobar")); } } TEST_F(RangerClientTest, BenchmarkAuthorizeActions) { Allow("jdoe", ActionPB::SELECT, "default", "foobar"); for (int i = 0; i < 1000000; ++i) { unordered_set<ActionPB, ActionHash> actions({ActionPB::SELECT}); ASSERT_OK(client_.AuthorizeActions("jdoe", "default.foobar", &actions)); } } http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/ranger/ranger_client.cc@234 PS23, Line 234: DCHECK_EQ(orig_table_names.size(), resp_list.responses_size()); : : unordered_set<string> allowed_tables; : for (auto i = 0; i < orig_table_names.size(); ++i) { : if (resp_list.responses(i).allowed()) { : EmplaceOrDie(&allowed_tables, move(orig_table_names[i])); : } : } : : if (allowed_tables.empty()) { : LOG(WARNING) << Substitute("User $0 is not authorized to perform $1 on $2 tables", : user_name, ActionPB_Name(action), table_names->size()); : return Status::NotAuthorized(kUnauthorizedAction); : } : : * > nit: may be simpler as Done http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/ranger/ranger_client.cc@295 PS23, Line 295: > nit: consider using JoinMapped() to output the actions instead of just the Done http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/ranger/ranger_client.cc@308 PS23, Line 308: _path; : : return Substitute("$0:$1", jar_path > nit: it'd be cleaner to read this without the negative. Also consider using Done http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/subprocess/subprocess_proxy.h File src/kudu/subprocess/subprocess_proxy.h: http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/subprocess/subprocess_proxy.h@83 PS23, Line 83: void ReplaceServerForTests(std::unique_ptr<SubprocessServer> server) { : server_ = std::move(server); > nit: It'd probably be less error-prone to pass a unique_ptr<> and std::move 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: 24 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 23:42:05 +0000 Gerrit-HasComments: Yes
