Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.h@223 PS3, Line 223: std::shared_ptr<AuthzInfoCache> cache_; > Should doc why it needs shared ownership. Done http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.h@226 PS3, Line 226: // of operations with cache items and concurrent request to reset the cache. > requests (to agree with plural of 'operations') Done http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.h@227 PS3, Line 227: // Not using std::atomic_load and std::atomic_exchange since those are: : // * have higher performance overhead for this particular case : // * not available while compiling on pre-RH7 platforms, even if using : // C++11-compatible compiler from thirdparty and devtoolset. > Nit: reword as: Done. The reworded version is more concise. Thanks! http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/sentry_privileges_fetcher.cc@453 PS1, Line 453: const auto& db = privilege_resp.dbName; > Are they really heavier-weight than acquiring a spinlock? * according to https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic#Notes , those are usually implemented using mutexes * yep, atomic-related for shared pointers works great with libc++ (at least on macOS), but on CentOS 6.6 there was an error I mentioned earlier Also, I took a look into thirdparty/src/llvm-6.0.0.src/projects/libcxx/include/memory: template <class _Tp> _LIBCPP_AVAILABILITY_ATOMIC_SHARED_PTR shared_ptr<_Tp> atomic_load(const shared_ptr<_Tp>* __p) { __sp_mut& __m = __get_sp_mut(__p); __m.lock(); shared_ptr<_Tp> __q = *__p; __m.unlock(); return __q; } Some details on __get_sp_mut from thirdparty/src/llvm-6.0.0.src/projects/libcxx/src/memory.cpp: __sp_mut& __get_sp_mut(const void* p) { static __sp_mut muts[__sp_mut_count] { &mut_back[ 0], &mut_back[ 1], &mut_back[ 2], &mut_back[ 3], &mut_back[ 4], &mut_back[ 5], &mut_back[ 6], &mut_back[ 7], &mut_back[ 8], &mut_back[ 9], &mut_back[10], &mut_back[11], &mut_back[12], &mut_back[13], &mut_back[14], &mut_back[15] }; return muts[hash<const void*>()(p) & (__sp_mut_count-1)]; } So, indeed: a global table hash-table consisting of 16 buckets, with mutex in each: _LIBCPP_SAFE_STATIC static const std::size_t __sp_mut_count = 16; _LIBCPP_SAFE_STATIC static __libcpp_mutex_t mut_back[__sp_mut_count] = { _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER }; >From thirdparty/src/llvm-6.0.0.src/projects/libcxx/include/__threading_support: typedef pthread_mutex_t __libcpp_mutex_t; http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.cc@487 PS3, Line 487: ResetCache(); > Curious why this call is here and not in the constructor? I tried to follow the pattern for sentry_client. I think we need it here anyways (even if adding ResetCache() into the constructor as well) since the semantics of Start/Stop propagated from SentryAuthzProvider assumes Sentry RPC address might change after stop, so it's important to clean the cache of possible irrelevant information. >From the other side, there is not much sense in an empty cache created in the >constructor before calling Start(). I'll add a comment. -- To view, visit http://gerrit.cloudera.org:8080/13063 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6 Gerrit-Change-Number: 13063 Gerrit-PatchSet: 3 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: Thu, 25 Apr 2019 21:24:52 +0000 Gerrit-HasComments: Yes
