Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 )
Change subject: [SentryAuthzProvider] deduplicate RPCs to Sentry ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/12967/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12967/1//COMMIT_MSG@7 PS1, Line 7: [SentryAuthzProvider] nit: seems like most of the heavy lifting is done in sentry_privileges_fetcher? http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_authz_provider-test.cc@906 PS1, Line 906: // If cache is not enabled and some threads are delayed with their call : // to SentryAuthzProvider up to the point when the first batch of requests : // has already been processed, some extra RPC(s) might be observed. : EXPECT_GE(kNumRequestThreads / 2, sentry_rpcs_num); Hrm, why isn't this EXPECT_EQ(kNumRequestThreads, sentry_rpcs_num)? With caching disabled, wouldn't every authorization call equate a call to Sentry? Same below http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc@437 PS1, Line 437: result Perhaps call this "fetched_privileges"? That way it's clear everywhere below what this is. http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc@440 PS1, Line 440: auto& info = LookupOrEmplace(&pending_requests_, : key, SentryRequestsInfo()) How about calling this PendingSentryRequests and 'pending_request'? 'info' is pretty nondescript as far as variable names go, so without going back to this line and the definition of SentryRequestsInfo, it isn't quite clear what this is IMO. http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc@446 PS1, Line 446: if (first_request) { : result = std::make_shared<SentryPrivilegesBranch>(); : info.result = result; : } else { : result = info.result; : } nit: slightly simpler as: if (first_request) { DCHECK(!info.result); info.result = std::make_shared<SentryPrivilegesBranch>(); } result = info.result; http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc@481 PS1, Line 481: if (s.ok()) { : *privileges = *result; : } : return s; nit: for parity with L453, may be written as: RETURN_NOT_OK(s); *privileges = *result; return Status::OK(); -- To view, visit http://gerrit.cloudera.org:8080/12967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 09 Apr 2019 19:04:47 +0000 Gerrit-HasComments: Yes
