Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries ...................................................................... Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/13196/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13196/1//COMMIT_MSG@7 PS1, Line 7: [util] interface to invalidate cache entries What motivated this patch? The CLI tool outright reset the entire TTL cache; is this an alternative to that approach? http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h File src/kudu/util/cache.h: http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@246 PS1, Line 246: typedef std::function<bool(size_t /* valid_entries_num */, I think it'd be simpler to combine this into the ValidityFunc. Invalidate callers that aren't interested in early out can ignore these two args. You can use an enum (or out param) to represent all of the "return states" of the functor. http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@251 PS1, Line 251: a an http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@252 PS1, Line 252: optional If it's optional, perhaps wrap it in boost::optional? http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@255 PS1, Line 255: when the provided functor is non-null Both functors are passed by cref so how could either be null? http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@256 PS1, Line 256: shard Sharding is an implementation detail and not a requirement for implementing this interface. Does this mean that all implementors must be sharded? If not, please find a way to express this without implying that implementors must shard. http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@260 PS1, Line 260: virtual size_t Invalidate(const ValidityFunc& validity_func, It's not clear from the commit message (or context) why all of this is useful: 1. Why do we need cache invalidation support? 2. Why is the short-circuit provided by IterationFunc useful? 3. Why does IterationFunc benefit from its two arguments? http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@268 PS1, Line 268: examing examine http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.cc File src/kudu/util/cache.cc: http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.cc@508 PS1, Line 508: std::lock_guard<MutexType> l(mutex_); If holding the mutex_ while calling the functors is a requirement, should document that in the header file (or something to that effect i.e. the functors must do a minimal amount of work because the cache may hold an internal lock while calling them). -- To view, visit http://gerrit.cloudera.org:8080/13196 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099 Gerrit-Change-Number: 13196 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 30 Apr 2019 22:34:39 +0000 Gerrit-HasComments: Yes
