Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13069 )
Change subject: [authz] new SentryAuthzProvider's caching strategy ...................................................................... Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/13069/4/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/13069/4/src/kudu/master/sentry_authz_provider-test.cc@729 PS4, Line 729: ASSERT_EQ(3, GetTasksSuccessful()); : : // All the requests below should also hit the cache since the information on : // the privileges granted on each of the tables in the requests below : // is in the cache. In the Sentry's privileges model for Kudu, DROP TABLE : // requires privileges on the table itself, but nothing is requred on the : // database the table belongs to. > I think it would be clearer because using Authorize doesn't presume knowled Yep, I added comments in attempt to clarify on the policies reflecting the Sentry's privileges model for Kudu. I think we better keep this as more 'synthetic' test to have stronger guarantees on the behavior of the cache in SentryPrivilegesFetcher. http://gerrit.cloudera.org:8080/#/c/13069/4/src/kudu/master/sentry_authz_provider-test.cc@776 PS4, Line 776: // while fetching the information privileges on 'db.t0' authorizable of the : // TABLE scope. : for (int idx = 0; idx < 10; ++idx) { : const auto table_name = Substitute("db1.t$0", idx); : ASSERT_OK(sentry_authz_provider_->AuthorizeCreateTable( : > Same here, I think it would be clearer because using Authorize doesn't pres Yep, I agree that would be a bit clearer and that make sense. However, I'd like to keep these tests more 'synthetic' so we are sure the cache behavior is reasonable and beneficial for the actual usage patterns we have in the code. http://gerrit.cloudera.org:8080/#/c/13069/4/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/13069/4/src/kudu/master/sentry_privileges_fetcher.cc@250 PS4, Line 250: key_sequence_max_size() method. > Ah, much clearer. Thanks! Yep, sure -- I'm working on the patch to get rid of SERVER-scope things scattered in a few places. Once they are gone and we have clear constraints on what we expect, I'll update this as well. I'm not sure I want to have it all in this changelist, even if it simplifies this code a bit. http://gerrit.cloudera.org:8080/#/c/13069/5/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/13069/5/src/kudu/master/sentry_privileges_fetcher.cc@252 PS5, Line 252: authorizable > nit: authorizables Done http://gerrit.cloudera.org:8080/#/c/13069/5/src/kudu/master/sentry_privileges_fetcher.cc@611 PS5, Line 611: if (requested_scope == SentryAuthorizableScope::DATABASE || : requested_scope == SentryAuthorizableScope::TABLE || : requested_scope == SentryAuthorizableScope::COLUMN) { > Once we have the CHECK_NE SERVER, we should be able to get rid of the condi Right, exactly. Thank you for the reminder. http://gerrit.cloudera.org:8080/#/c/13069/5/src/kudu/master/sentry_privileges_fetcher.cc@633 PS5, Line 633: requested_scope == SentryAuthorizableScope::COLUMN) { > This shouldn't be possible from L538, right? Same above. This is still possible in a RELEASE build. Do you think we want to convert DCHECK at line L538 into a CHECK? We might allow COLUMN requests with any issues -- the scope is artificially widened into TABLE at L541. -- To view, visit http://gerrit.cloudera.org:8080/13069 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id96181345e357a104e28314d8d8d88633dcf9608 Gerrit-Change-Number: 13069 Gerrit-PatchSet: 6 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-Comment-Date: Tue, 23 Apr 2019 18:28:28 +0000 Gerrit-HasComments: Yes
