Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 )
Change subject: WIP [master] introduced SentryAuthzCache ...................................................................... Patch Set 5: (11 comments) http://gerrit.cloudera.org:8080/#/c/12833/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12833/2//COMMIT_MSG@7 PS2, Line 7: WIP [master] introduced SentryAuthzCache > Any thoughts on tackling the thundering herd calls to Sentry in this patch? I'm planning to handle the thundering herd will be in a follow-up patch, since that's an optimization. http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_cache.h File src/kudu/master/sentry_authz_cache.h: http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_cache.h@56 PS2, Line 56: : : : > Is synchronization built into TTLCache? Do we need to worry about concurren It is -- the underlying FIFO cache is thread-safe, do I don't think we need to worry about that stuff here. I'll add corresponding comments. Also, I think to rename this into SentryAuthzFetcher and move the whole cached/non-cached if/else code fromk sentry_authz_provider.cc here. http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider-test.cc@104 PS2, Line 104: TDropSentryRoleRequest role_req; : role_req.__set_requestorUserName(kAdminUser); : role_req.__set_roleName(role_name); > nit: why not Ah, that's just some by-product of different code composition when NotifyAuthzInfoUpdated. Of course, regular RETURN_NOT_OK() will work here. http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider.h@105 PS2, Line 105: const ::sentry::TSentryPrivilege& privilege, : const ::sentry::TSentryAuthorizabl > Could you elaborate on what this means? Why do we need a method for this (e That was an evolutionary step, yup. Now I think it's better to call ResetCache() from the test itself, right. http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider.cc@120 PS2, Line 120: : > nit: reword as, "0 means Sentry responses will not be cached." Done http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc@111 PS4, Line 111: TAG_FLAG(sentry_service_max_message_size_bytes, advanced); > How did you arrive at this default value? Nothing particular: some random value. I think we need to make some reasonable default value here. I added TODO. http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc@116 PS4, Line 116: DEFINE_uint32(sentry_authz_cache_capacity_bytes, 256 * 1024 * 1024, > Of the new flags, this one doesn't necessarily seem experimental. Done http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc@331 PS4, Line 331: CHECK(!key_sequence_.empty()); > Would be more efficient to break this out into the N possible cases and do All right, I put up something using Substitute(). Hopefully, it will be better from performance perspective. However, I think at this point it's more about speculating whether this or that is more efficient -- I didn't do any performance comparisons. http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc@369 PS4, Line 369: for (const auto& p : obj->privileges) { > Instead of this, pass in the pointer itself and call kudu_malloc_usable_siz Done http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc@530 PS4, Line 530: ++idx; > Could you allocate an empty TListSentryPrivilegesResponse here, and use it Done http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc@582 PS4, Line 582: } else { > Seems like you want to call cache_.reset() either way if the goal is to acc That would be safer, yep. -- To view, visit http://gerrit.cloudera.org:8080/12833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe Gerrit-Change-Number: 12833 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Wed, 27 Mar 2019 06:54:31 +0000 Gerrit-HasComments: Yes
