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 <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: 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-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 10 Apr 2019 16:02:58 +0000 Gerrit-HasComments: Yes
