Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache ...................................................................... Patch Set 13: (18 comments) Addressed the first batch of comments, I'll address the next batch of comments in the next revision. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc File src/kudu/util/slru_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc@75 PS12, Line 75: lic Cac > What's the unit for the 'size'? Maybe add a small doc blurb about that, do Do you mean for 'probationary_segment_size' and 'protected_segment_size'? Just size_t which is an unsigned long I believe. Maybe I don't fully understand the question. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc@90 PS12, Line 90: > nit: indeed, -1 is quite a special value :) Do you think it's safer to use I think it's ok to use -1 here, I'll add a comment noting how -1 is used as the default value for no value was found. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc@170 PS12, Line 170: > nit: would be great to add a small doc blurb to explain why 1/3 is the pref I'm not sure what the preferred ratio is, I just chose a ratio that I thought would make sense here. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc@193 PS12, Line 193: Test : > nit for here and above: maybe, create two constants out of these two argume Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc@447 PS12, Line 447: ASSERT_FALSE(slru_cache_->ProbationaryContains(EncodeInt(i))); > Please add a few test scenarios to make sure the code works as expected at Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@71 PS12, Line 71: } > nit: add a comment as for the other methods in this interface? Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@291 PS12, Line 291: } > Is it OK to override an already set eviction callback for a handle? If not This is a newly allocated handle, so it won't have an eviction callback set until this line. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@573 PS12, Line 573: > nit: the number Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@573 PS12, Line 573: > nit: is? Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@574 PS12, Line 574: kup > nit: is ? Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@574 PS12, Line 574: acheSh > nit: the number Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@583 PS12, Line 583: // segment. Return the lookup handle. If any entries are evicted f > Does it make sense to optimize for the case when the entry is already in th I thought about this as well, the reason I called both Lookup methods was to update the lookup metrics for both segments. I guess I could simply call UpdateLookupMetrics for the probationary shard before returning the protected segment handle. That would help increase the concurrency too as less time would be spent with the probationary shard locked. The one drawback would this approach is we wouldn't be able to check the invariant that both segments shouldn't contain the same entry. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@595 PS12, Line 595: > nit: isn't ? Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@596 PS12, Line 596: > return the entry found in the probationary segment Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@596 PS12, Line 596: > nit: the protected segment Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@608 PS12, Line 608: return probationary_handle; > What if the entry that has just been inserted evicted right away with all t So this would only happen in the case that the entry being upgraded is larger than the capacity limit of the protected segment IIUC. It would also be then be part of 'evictions' here and re-inserted into the probationary segment. There would be no way for this entry to be protected if its charge is larger than the capacity of the protected segment. In this case, perhaps it would be better to check if the entry's charge is larger than the capacity before adding it to the segment as that would evict all the other entries from the protected segment for no reason. I didn't think about this edge case, thanks for bringing it up, I'll add a test case for it. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@610 PS12, Line 610: > nit: the protected segment Done http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@612 PS12, Line 612: // Erase entry from the probationary segment th > Could use reverse iterator instead of dancing with indices? Done -- To view, visit http://gerrit.cloudera.org:8080/20607 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45531534a2049dd38c002f4dc7982df9fd46e0bb Gerrit-Change-Number: 20607 Gerrit-PatchSet: 13 Gerrit-Owner: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 15 Feb 2024 23:12:50 +0000 Gerrit-HasComments: Yes
