[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. [SentryPrivilegesFetcher] deduplicate RPCs to Sentry With this patch, concurrent requests to Sentry with the same set of parameters are queued, so the actual number of RPC requests sent to Sentry is reduced. Added a couple of tests to cover the newly added functionality. Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Reviewed-on: http://gerrit.cloudera.org:8080/12967 Tested-by: Alexey Serbin Reviewed-by: Hao Hao Reviewed-by: Andrew Wong --- M src/kudu/master/CMakeLists.txt M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_privileges_fetcher.cc M src/kudu/master/sentry_privileges_fetcher.h 4 files changed, 197 insertions(+), 20 deletions(-) Approvals: Alexey Serbin: Verified Hao Hao: Looks good to me, approved Andrew Wong: Looks good to me, approved -- 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: merged Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12967 )
Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12967/4/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/12967/4/src/kudu/master/sentry_privileges_fetcher.cc@355
PS4, Line 355: if (VLOG_IS_ON(1)) {
: std::ostringstream os;
: privilege_resp.printTo(os);
: if (action != SentryAction::ALL && action !=
SentryAction::OWNER &&
: privilege_resp.grantOption ==
TSentryGrantOption::ENABLED) {
: VLOG(1) << "ignoring ENABLED grant option for unexpected
action: "
: << static_cast(action);
: }
> Whoops, just noticed this upon looking at the 3->4 interdiff, this is alway
Good catch! I'll take care of that in a follow-up patch to address Hao's
request to add an extra scenario for the concurrent requests.
I thought the compiler would issue a warning, but may be we have those
particular warnings disabled.
--
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: 4
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 11 Apr 2019 01:15:53 +
Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/12967 )
Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
..
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12967/4/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/12967/4/src/kudu/master/sentry_privileges_fetcher.cc@355
PS4, Line 355: if (VLOG_IS_ON(1)) {
: std::ostringstream os;
: privilege_resp.printTo(os);
: if (action != SentryAction::ALL && action !=
SentryAction::OWNER &&
: privilege_resp.grantOption ==
TSentryGrantOption::ENABLED) {
: VLOG(1) << "ignoring ENABLED grant option for unexpected
action: "
: << static_cast(action);
: }
Whoops, just noticed this upon looking at the 3->4 interdiff, this is always
filling an ostringstream and not doing anything with it. Should probably be:
if (VLOG_IS_ON(1) &&
action != SentryAction::ALL && action != SentryAction::OWNER &&
privilege_resp.grantOption == TSentryGrantOption::ENABLED) {
VLOG(1) << "ignoring enabled grant option for action: " <<
ActionToString(action);
}
or just get rid of it; having a grant option for a non-ALL/OWNER action isn't
interesting to know about.
Feel free to do this separately though, since it's not part of this patch.
--
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: 4
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 11 Apr 2019 01:11:56 +
Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Patch Set 4: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc@1010 PS3, Line 1010: est scenario wit > Sure, but I'd like to add it in a separate changelist, otherwise chances of Sure, LGTM. Thanks! -- 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: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 11 Apr 2019 00:17:31 + Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Patch Set 4: Verified+1 unrelated flake in RemoteKsckTest.TestClusterWithLocation -- 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: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 10 Apr 2019 23:45:19 + Gerrit-HasComments: No
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Alexey Serbin has removed a vote on this change. Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Removed Verified-1 by Kudu Jenkins (120) -- 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: deleteVote Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12967 to look at the new patch set (#4). Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. [SentryPrivilegesFetcher] deduplicate RPCs to Sentry With this patch, concurrent requests to Sentry with the same set of parameters are queued, so the actual number of RPC requests sent to Sentry is reduced. Added a couple of tests to cover the newly added functionality. Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef --- M src/kudu/master/CMakeLists.txt M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_privileges_fetcher.cc M src/kudu/master/sentry_privileges_fetcher.h 4 files changed, 197 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12967/4 -- 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: newpatchset Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Patch Set 4: (5 comments) 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: SERT_EQ(2, GetReconnectionsSucceeded()); : : // NotAuthorized() from Sentry itself considered as a fatal error. : // TODO(KUDU-2769): clarify whether it is a bug in Ha > Ah I see. Did you loop this with stress to arrive at kNumThreads/2 ? Nope, I just put that as some 'reasonable enough' criteria. But I did run this scenario in loop mode with --stress-cpu-threads=16 and verified that it passes with no issues for TSAN builds (~256 runs). http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc@998 PS3, Line 998: questThreads / 2) of actual RPC request > Can you explain a bit more on what kind of scheduling anomalies can happen? Done http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc@1010 PS3, Line 1010: est scenario wit > Can you add a test case when the Sentry server is not reachable? Sure, but I'd like to add it in a separate changelist, otherwise chances of conflicts are increasing, and I spent considerable amount of time resolving those already. Will it be OK with you if I add that in a follow-up changelist? http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.h@237 PS3, Line 237: > Remove? Done http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.cc@154 PS3, Line 154: using std::string; > nit: this should be in L153? Indeed; re-ordered. -- 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: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 10 Apr 2019 22:57:51 + Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc@998 PS3, Line 998: kNumRequestThreads / 2, sentry_rpcs_num Can you explain a bit more on what kind of scheduling anomalies can happen? http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc@1010 PS3, Line 1010: FailureResponses Can you add a test case when the Sentry server is not reachable? http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.h@237 PS3, Line 237: //std::unordered_map pending_requests_; Remove? http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.cc@154 PS3, Line 154: using std::shared_ptr; nit: this should be in L153? -- 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: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 10 Apr 2019 19:29:36 + Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Patch Set 3: Code-Review+1 (1 comment) LGTM, just one question. 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: SERT_EQ(2, GetReconnectionsSucceeded()); : : // NotAuthorized() from Sentry itself considered as a fatal error. : // TODO(KUDU-2769): clarify whether it is a bug in Ha > Actually, in both cases it should be ASSERT_GE(...). The statement about g Ah I see. Did you loop this with stress to arrive at kNumThreads/2 ? -- 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: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 10 Apr 2019 18:27:56 + Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Patch Set 3: Code-Review+1 -- 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: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 10 Apr 2019 16:43:30 + Gerrit-HasComments: No
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12967 )
Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
..
Patch Set 2:
(15 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_fetc
Done
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 ca
Actually, in both cases it should be ASSERT_GE(...). The statement about
guaranteed single RPC request to Sentry in case if the caching is enabled is
wrong -- see Adar's comment and my response to that in PS2.
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_authz_provider-test.cc@884
PS2, Line 884: vector threads;
> Might find it easier to use a ThreadPool for this, especially since the def
I guess ThreadPool is harder to me than simple std::thread in this context :)
Testing it for various number of threads sounds like a good idea, especially to
cover the case when requests to SentryAuthzProvider come from Kudu's
ThreadPool. I added parameterisation for that.
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_authz_provider-test.cc@903
PS2, Line 903: // If cache is enabled, it's guaranteed to have a single RPC
sent to Sentry.
> I don't see how this is the case. Couldn't a particularly pathological seri
Ah, right. It seems that I confused current implementation with some other
wild idea I had: using the cache to store information about pending requests
along with the response (if any). But the cache's interface didn't allow for
achieving that way of synchronization, in fact.
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_authz_provider-test.cc@947
PS2, Line 947: is not storing
> Nit: does not store
Done
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.h
File src/kudu/master/sentry_privileges_fetcher.h:
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.h@234
PS2, Line 234: parametes
> parameters
Done
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:
> Perhaps call this "fetched_privileges"? That way it's clear everywhere belo
Done
http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc@440
PS1, Line 440: sentry_client_.Stop();
: }
> How about calling this PendingSentryRequests and 'pending_request'? 'info'
Done. I thin naming the variable as 'pending_request' suffices.
http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc@446
PS1, Line 446: scope_depth_limit_.reset();
: } else {
: SentryAuthorizableScope deepest_scope;
: RETURN_NOT_OK(SentryAuthorizableScope::FromString(
: scope_level_limit_str, _scope));
: s
> nit: slightly simpler as:
Done
http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc@481
PS1, Line 481: *privileges = handle.value();
: return Status::OK();
: }
:
> nit: for parity with L453, may be written as:
Done
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.cc@484
PS2, Line 484:
> Should explain why we need to wrap result in shared_ptr.
Done
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.cc@485
PS2, Line 485: Synchronizer sync;
> How many usages of this pattern do we have now? I can think of another one
All right, I'll open a JIRA for that.
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.cc@487
PS2, Line 487: std::shared_ptr result;
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12967 to look at the new patch set (#3). Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. [SentryPrivilegesFetcher] deduplicate RPCs to Sentry With this patch, concurrent requests to Sentry with the same set of parameters are queued, so the actual number of RPC requests sent to Sentry is reduced. Added a couple of tests to cover the newly added functionality. Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef --- M src/kudu/master/CMakeLists.txt M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_privileges_fetcher.cc M src/kudu/master/sentry_privileges_fetcher.h 4 files changed, 186 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12967/3 -- 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: newpatchset Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
