Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries ...................................................................... Patch Set 1: (12 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 Ah, it seems I cut corners with commit messages while trying to put up a patch for review sooner :) I added more information into the commit message. Thank you for the prompt review! http://gerrit.cloudera.org:8080/#/c/13196/1//COMMIT_MSG@7 PS1, Line 7: [util] interface to invalidate cache entries > Answering my own question: this supports the TTL cache scrubbing thread. Co Done 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 c I seems to me that keeping ValidityFunc and IterationFunc orthogonal is a bit clearer. I tried to go with that combined-enum approach for return type of would-be-validity-functor, and I didn't like it. I opted for having InvalidationControl stucture with default parameters for its constructor. I found it easier to use than a method with two parameters. http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@251 PS1, Line 251: a > an Done 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? Ah, that's outdated. Originally, it was optional since it was a parameter with default value. However, code style doesn't allow parameters by default in signatures of virtual functions. I updated the comment. 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? My bad -- this comment is outdated. 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 Ah, that was an explicit attempt to avoid possible backlash due to 'vagueness'. All right, I think I can make it generic and use a sharded cache as an example. 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 usef I added more information into the commit message and more in-line doc for the IterationFunc. http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@268 PS1, Line 268: examing > examine Done 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@64 PS1, Line 64: Slice, Slice) { > warning: all parameters should be named in a function [readability-named-pa Done http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.cc@69 PS1, Line 69: size_t, size_t) { > warning: all parameters should be named in a function [readability-named-pa Done 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 d I added a comment in the in-line doc for new methods in cache.h -- 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: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 01 May 2019 07:20:37 +0000 Gerrit-HasComments: Yes
