Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13494 to look at the new patch set (#6). Change subject: sentry: don't send requests for DATABASE/SERVER privileges ...................................................................... sentry: don't send requests for DATABASE/SERVER privileges The API used to request privileges from Sentry will return more privileges than required for a given request. For example, if requesting privileges for a specific database, the API will return privileges for all ancestors and all descendents of that database, i.e. privileges for the server, the database, all tables in the database, and all columns therein. To remediate this, this patch narrows the scope of requests sent to Sentry to the table-level. Table-level seems fitting since every request for privileges Kudu makes to Sentry thus far is naturally scoped to a table anyway. Note: this patch doesn't touch any of the caching logic; it only tweaks the request sent to Sentry. I wrote a small benchmark[1] to gauge the effects of this patch. The time spent checking database-level privileges with 20K privilege entries in the database was 1.989 without this patch, and 0.491s with this patch. W0602 07:44:03.110574 13909 sentry_client.cc:188] Time spent alter Sentry role grant privileges: real 3.861s user 0.000s sys 0.000s I0602 07:44:03.110697 13909 sentry_authz_provider-test.cc:739] num tables granted: 19998 W0602 07:44:06.829799 13909 sentry_client.cc:188] Time spent alter Sentry role grant privileges: real 3.719s user 0.000s sys 0.000s I0602 07:44:06.829936 13909 sentry_authz_provider-test.cc:739] num tables granted: 19999 W0602 07:44:10.521934 13909 sentry_client.cc:188] Time spent alter Sentry role grant privileges: real 3.692s user 0.000s sys 0.000s W0602 07:44:12.375082 14301 sasl_client_transport.cc:160] Received large Thrift SASL frame: 2.33M W0602 07:44:12.438380 14301 sentry_client.cc:170] Time spent list Sentry privileges by user: real 1.892s user 0.054s sys 0.009s W0602 07:44:12.509138 13909 sentry_authz_provider.cc:275] Action <CREATE> on table <dbwithalongname.table> with authorizable scope <DATABASE> is not permitted for user <test-user> I0602 07:44:12.510848 13909 sentry_authz_provider-test.cc:744] Time spent Getting database privileges: real 1.989s user 0.070s sys 0.002s W0602 07:44:13.002019 13909 sentry_authz_provider.cc:275] Action <CREATE> on table <dbwithalongname.table> with authorizable scope <DATABASE> is not permitted for user <test-user> I0602 07:44:13.002111 13909 sentry_authz_provider-test.cc:750] Time spent Getting database privileges: real 0.491s user 0.000s sys 0.000s While the test numbers themselves may not be representative of a real Sentry deployment (since the test uses a Sentry minicluster instead of a real cluster) the numbers still point to reasonable improvement. A version of this benchmark is included in this patch. I opted to not gate the behavior behind a gflag, so rather than running successive privileges with different behavior, the included test only checks privileges once. [1] https://gist.github.com/andrwng/88f3a760441b4b37b3ff011e656683ab Change-Id: Ic0025e3bacc8449dfffe99a1fc062a9e6787eb78 --- 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, 70 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/13494/6 -- To view, visit http://gerrit.cloudera.org:8080/13494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic0025e3bacc8449dfffe99a1fc062a9e6787eb78 Gerrit-Change-Number: 13494 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong <aw...@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)