You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/04/09 07:55:05 UTC
[kudu-CR] [SentryAuthzProvider] deduplicate RPCs to Sentry
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12967
Change subject: [SentryAuthzProvider] deduplicate RPCs to Sentry
......................................................................
[SentryAuthzProvider] 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/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/master/sentry_privileges_fetcher.cc
M src/kudu/master/sentry_privileges_fetcher.h
5 files changed, 184 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12967/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: newchange
Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef
Gerrit-Change-Number: 12967
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 10 Apr 2019 23:45:19 +0000
Gerrit-HasComments: No
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 10 Apr 2019 16:43:30 +0000
Gerrit-HasComments: No
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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<int16_t>(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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 11 Apr 2019 01:15:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 11 Apr 2019 00:17:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Reviewed-by: Hao Hao <ha...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [SentryAuthzProvider] deduplicate RPCs to Sentry
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 09 Apr 2019 19:04:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 10 Apr 2019 22:57:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 10 Apr 2019 18:27:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [SentryAuthzProvider] deduplicate RPCs to Sentry
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 )
Change subject: [SentryAuthzProvider] deduplicate RPCs to Sentry
......................................................................
Patch Set 2:
(8 comments)
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<thread> threads;
Might find it easier to use a ThreadPool for this, especially since the default number of threads == ncpus, which is probably what you want anyway.
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 series of scheduling decisions yield requests that all arrive together (and thus all miss in the cache) but are drawn out thereafter, leading to one Sentry RPC per request?
Seems like the same is possible on L909 too, but since the EXPECT is fairly permissive it's probably less likely.
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
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
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.
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 in KuduClient::Data::ConnectToClusterAsync.
If we have three or more, perhaps we should look into consolidating it as a standalone class in util.
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.cc@487
PS2, Line 487: std::shared_ptr<SentryPrivilegesBranch> result;
using
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.cc@496
PS2, Line 496: if (first_request) {
: result = std::make_shared<SentryPrivilegesBranch>();
: info.result = result;
: } else {
: result = info.result;
: }
Can this be rewritten:
if (first_request) {
info.result = std::make_shared<>();
}
result = info.result;
--
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: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 09 Apr 2019 19:47:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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<std::string, SentryRequestsInfo> 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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 10 Apr 2019 19:29:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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<thread> 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, &deepest_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<SentryPrivilegesBranch> result;
> using
Done
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.cc@496
PS2, Line 496: if (first_request) {
: result = std::make_shared<SentryPrivilegesBranch>();
: info.result = result;
: } else {
: result = info.result;
: }
> Can this be rewritten:
Done
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.cc@516
PS2, Line 516: memory_footprint(result_ptr.get()) + key.capacity();
> error: use of undeclared identifier 'memory_footprint' [clang-diagnostic-er
Done
--
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: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 10 Apr 2019 16:02:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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<int16_t>(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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 11 Apr 2019 01:11:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [SentryAuthzProvider] deduplicate RPCs to Sentry
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, 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 (#2).
Change subject: [SentryAuthzProvider] deduplicate RPCs to Sentry
......................................................................
[SentryAuthzProvider] 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/sentry_authz_provider-test.cc
M src/kudu/master/sentry_privileges_fetcher.cc
M src/kudu/master/sentry_privileges_fetcher.h
3 files changed, 177 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12967/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: newpatchset
Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef
Gerrit-Change-Number: 12967
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)