Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread ...................................................................... Patch Set 7: Code-Review+2 (4 comments) LGTM, just minor nits that I don't feel particularly strongly about if you disagree. http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc File src/kudu/util/ttl_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@483 PS5, Line 483: } : // Now, once the handles for the selected entries are gone out of scope, : // the cache should be empty. : ASSERT_EQ(kHalfCacheCapacity, GetCacheEvictionsExpired()); : ASSERT_EQ(0, GetCacheUsage()); : : // Make sure the scrubber doesn't conflate non-expired entries with expired : // ones while scubbing the cache. : for (auto i = 0; i < kHalfCacheCapacity; ++i) { : cache.Put(Substitute("1$0", i), unique_ptr<TestValue>(new TestValue), 1); : } : SleepFor(kEntryTtlThreeQuarters); : for (auto i = 0; i < kHalfCacheCapacity; ++i) { : cache.Put(Substitute("2$0", i), unique_ptr<TestValue>(new TestValue), 1); : } : ASSERT_EQ(kCacheCapacity, GetCacheUsage()); : SleepFor(kEntryTtlThreeQuarters); > What hasn't been covered yet: the scrubbing thread doesn't conflate expired Gotcha, thanks for the explanation. It's much clearer now. nit: it might be even clearer to separate this out into it's own test, so the ASSERT_EQ(kHalfCacheCapacity, GetCacheEvictionsExpired()); wouldn't depend on that fact that we expired half the cache in the previous part of the test, and this functionality could be reasoned about in isolation. http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc File src/kudu/util/ttl_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@523 PS7, Line 523: const auto kNumRunnerThreads = AllowSlowTests() ? 64 : 16; Just making sure I understand why this is important. With more threads, we'll expect more handles in the cache active at any given time, and we might expect invalidation to avoid invalidating these "active" handles. Is that right? http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@544 PS7, Line 544: + thread_idx nit: isn't rand() enough to sufficiently differentiate the sleeps here? Is this important? If not, we don't have to copy in the thread_idx http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@548 PS7, Line 548: SCOPED_CLEANUP({ : stop = true; : for (auto& thread : threads) { : thread.join(); : } : }); nit: maybe this should go above the emplacement of the threads, in case thread creation fails for some reason? Though probably not important since I think all the threads will be cleaned up anyway. -- To view, visit http://gerrit.cloudera.org:8080/13201 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c Gerrit-Change-Number: 13201 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 02 May 2019 20:30:02 +0000 Gerrit-HasComments: Yes
