Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12652 )
Change subject: [master] introduce TTL-based cache ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h File src/kudu/master/ttl_cache.h: PS1: This probably belongs in /util, given there's nothing master-specific about it. Also maybe pull some of this out into a cc file? Also, please add function-level comments on how a user should expect the public API to work wrt TTLs, eviction, etc. http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@54 PS1, Line 54: const auto lookup_it = lookup_map_.find(key); nit: FindOrNull? Below too. http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@81 PS1, Line 81: if (value) { Do you expect to use this outside of test? If so, could you add a function-level comment indicating this is allowed by the Get API? http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@88 PS1, Line 88: lookup_map_.erase(lookup_it); I found this part of the API is kind of surprising. I would have expected the TTL would prevent us from returning an expired entry. What's the rationale behind returning an expired entry? Also probably worth adding the behavior to the function-level comment. http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@100 PS1, Line 100: contents of nit: number of elements in http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@105 PS1, Line 105: lookup_map_.size() + 1 <= capacity_ nit: writing this without the addition (very slightly) reduces the cycles it takes to understand this. http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@108 PS1, Line 108: while (lookup_map_.size() >= capacity_ && !lookup_map_.empty()) { : const auto& oldest_entry = entry_list_.back(); : CHECK_EQ(1, lookup_map_.erase(oldest_entry.first)); : entry_list_.pop_back(); : } If you did a D/CHECK that (lookup_map_.size() == capacity_), you could get rid of the loop. http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@124 PS1, Line 124: std::list<std::pair<K, V>> entry_list_; Add a comment indicating that the cache maintains that this list is sorted by expiration time of its expiration time. -- To view, visit http://gerrit.cloudera.org:8080/12652 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If214fe19810728fa590d1e0a3e5b2da86b020ac2 Gerrit-Change-Number: 12652 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 04 Mar 2019 19:42:26 +0000 Gerrit-HasComments: Yes
