Adar Dembo 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. 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') 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: An alternative would be to use std::atomic_load and std::atomic_exchange, but: * They're higher overhead operations than just using a spinlock. * They're not available in all C++11-compatible compilers. 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; > I looked at them and there are a couple of things: Are they really heavier-weight than acquiring a spinlock? Anyway, unavailability in devtoolset3 is certainly a deal-breaker. 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? -- 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 <[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-Comment-Date: Thu, 25 Apr 2019 20:25:19 +0000 Gerrit-HasComments: Yes
