Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread ...................................................................... Patch Set 5: (5 comments) 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: : for (auto i = 0; i < cache_capacity / 2; ++i) { : cache.Put(Substitute("1$0", i), unique_ptr<TestValue>(new TestValue), 1); : } : SleepFor(scrub_interval); : for (auto i = 0; i < cache_capacity / 2; ++i) { : cache.Put(Substitute("2$0", i), unique_ptr<TestValue>(new TestValue), 1); : } : ASSERT_EQ(cache_capacity, GetCacheUsage()); : : SleepFor(entry_ttl); : : // Apart from the OS scheduler's anomalies, the scrubbing thread should run : // at least once after at this point, and at least half of the entries : // in the cache should have been invalidated by the time it ran. : ASSERT_LE(cache_capacity, GetCacheEvictionsExpired()); : ASSERT_GE(cache_capacity / 2, GetCacheUsage()); > Gotcha, thanks for the explanation. It's much clearer now. Done 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@442 PS7, Line 442: > nit: indent. Done http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@523 PS7, Line 523: > Just making sure I understand why this is important. With more threads, we' I was not sure whether 64 is a good number of threads by default, but I wanted to have it covered as well, but paremeterizing on that didn't look like something's worth pursuing. The difference in run-time is not big, though -- it's just in order of milliseconds. Probably, I'll better enable 64 threads for DEBUG/RELEASE builds and keep 16 for TSAN/ASAN. http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@544 PS7, Line 544: > nit: isn't rand() enough to sufficiently differentiate the sleeps here? Is Done http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@548 PS7, Line 548: : : : : : > nit: maybe this should go above the emplacement of the threads, in case thr Done -- 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: 5 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: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 02 May 2019 23:53:00 +0000 Gerrit-HasComments: Yes
