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: (10 comments) http://gerrit.cloudera.org:8080/#/c/13069/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13069/5//COMMIT_MSG@87 PS5, Line 87: irrelevant information on non-Kudu : tables sent by Sentry with a _database-scope_ response > Might worth a test for this as well? That's a good idea. I added the test. http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_authz_provider-test.cc@708 PS6, Line 708: requried > nit: required Done http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_authz_provider-test.cc@734 PS6, Line 734: requred > nit: required Done http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_authz_provider-test.cc@776 PS6, Line 776: db > nit: still a bit confused here, do you mean 'db' in L780? Because you are d Fixed, thanks for pointing that out. http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_privileges_fetcher.h@126 PS6, Line 126: : // Add/merge privileges from other instance of SentryPrivilegesBranch. : void Merge(const SentryPrivilegesBranch& other); : : // Output the privileges into branches corresponding to DB-and-higher and : // TABLE-and-lower authz scopes. : void Split(SentryPrivilegesBranch* other_scope_db, : SentryPrivilegesBranch* other_scope_table) const; > What do you think about making these operations destructive? Meaning: Yep, I thought about that already. * making Split() destructive doesn't seem a good alternative to me: it would change the shared response received from Sentry that might be used by other concurrent threads * making Merge destructive doesn't look like a good idea since it works with cache items, and moving privileges from 'other' means messing up the information stored in the cache. As an alternative, it would make sense to store the same entry in the cache for both TABLE and DATABASE (if received as the response for the TABLE-scope authorizable), but since TTLCache doesn't have any extra-wrapper (e.g., like FileCache), that's not the case: the entry ownership for the underlying Cache must follow the 'unique' pattern. Yes, at this point we do a lot of copying here, but I'm not sure this is a crucial point to address right now. http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_privileges_fetcher.cc@225 PS6, Line 225: GetFlattenedKey > nit: can you add a comment here? Done http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_privileges_fetcher.cc@233 PS6, Line 233: constexpr static size_t key_sequence_max_size() { > I think this would be more clear if defined as a constant rather than hidde Done http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_privileges_fetcher.cc@234 PS6, Line 234: 2 > nit: can you briefly explain why it is 2 now? Done http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_privileges_fetcher.cc@422 PS6, Line 422: scope_db.privileges_.push_back(e); > Let's use emplace_back() here and on L426 for new code. I might be missing something here: I'm not sure that's what we want here: emplace_back() with std::move() is not an option (see my response to your comment w.r.t. moving instead of copying in Merge() and Split() methods); and other cases would require specifying parameters for the constructors otherwise. http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_privileges_fetcher.cc@529 PS6, Line 529: uncomment when it's guaranteed we are not getting : // requests for authorizables of scope wider than DATABASE. > We don't have any Server level authorizables request today? Or there are so We do. Yes, those calls are from tests. -- 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: Wed, 24 Apr 2019 02:47:05 +0000 Gerrit-HasComments: Yes
