Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12652 )
Change subject: [util] introduce TTL-based cache ...................................................................... Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache-test.cc File src/kudu/master/ttl_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache-test.cc@79 PS1, Line 79: > Could you add a comment explaining why you expect cache size to be 5 here? Sure -- that's because cache's size is always capped by its capacity parameter regardless number of Put() calls. Done. http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache-test.cc@85 PS1, Line 85: > nit, typo: "should"? Done 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 Done. The code should be in a .h file since it's a class template. http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@45 PS1, Line 45: > I wonder if there is a legitimate case for 0-capacity cache, such as disabl 0-capacity cache is not legitimate. Yes, there is a way to disable the cache: see https://gerrit.cloudera.org/#/c/12653/1/src/kudu/master/sentry_authz_provider.cc@152 I prefer to use explicit semantics. http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@54 PS1, Line 54: > nit: FindOrNull? Below too. Done http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@81 PS1, Line 81: > Do you expect to use this outside of test? If so, could you add a function- Done http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@88 PS1, Line 88: > I found this part of the API is kind of surprising. I would have expected t Yep, that approach differs from cache to cache. I'll change it to reflect the most common expectations. http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@100 PS1, Line 100: > nit: number of elements in Done http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@105 PS1, Line 105: > nit: writing this without the addition (very slightly) reduces the cycles i Whoops, I'm surprised I wrote that. http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@108 PS1, Line 108: : : : : > If you did a D/CHECK that (lookup_map_.size() == capacity_), you could get That's a good idea -- my first approach was to write some generic method, but I agree there is no need for that in this implementation. http://gerrit.cloudera.org:8080/#/c/12652/1/src/kudu/master/ttl_cache.h@124 PS1, Line 124: > Add a comment indicating that the cache maintains that this list is sorted Done -- 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: 2 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: Greg Solovyev <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 11 Mar 2019 23:32:07 +0000 Gerrit-HasComments: Yes
