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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 09 Apr 2019 19:47:29 +0000 Gerrit-HasComments: Yes
