Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache ...................................................................... Patch Set 12: (18 comments) I took a quick look and left a few comments. I'm going to continue tomorrow, but if it makes sense, you might consider addressing the items that I already pointed at. Thank you! 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: lookups What's the unit for the 'size'? Maybe add a small doc blurb about that, documenting the constructor. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc@90 PS12, Line 90: -1; nit: indeed, -1 is quite a special value :) Do you think it's safer to use std::optional instead of introducing the special value semantics? Or the performance here is the priority? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc@170 PS12, Line 170: 4 * 1024 * 1024, 12 * 1024 * 1024 nit: would be great to add a small doc blurb to explain why 1/3 is the preferred ratio for the tests. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc@193 PS12, Line 193: 4 * 1024 * 1024, 12 * 1024 * 1024 nit for here and above: maybe, create two constants out of these two arguments and use those instead? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc@447 PS12, Line 447: } // namespace kudu Please add a few test scenarios to make sure the code works as expected at least for a few corner cases, such as: * trying to insert an entry that's larger than the capacity of the cache's sections: both, greater than just the probationary segment * inserting an entry that isn't in the cache when the probationary segment is exactly at capacity, and the entry being inserted is exactly of the probationary segment's side 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: Cache::Handle* Insert(Cache::RLHandle* handle, Cache::EvictionCallback* eviction_callback); nit: add a comment as for the other methods in this interface? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@291 PS12, Line 291: handle->eviction_callback = eviction_callback; Is it OK to override an already set eviction callback for a handle? If not, maybe add corresponding DCHECK() here? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@573 PS12, Line 573: are nit: is? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@573 PS12, Line 573: number nit: the number http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@574 PS12, Line 574: are nit: is ? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@574 PS12, Line 574: number nit: the number http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@583 PS12, Line 583: Cache::Handle* protected_handle = protected_shard_->Lookup(key, hash, caching); Does it make sense to optimize for the case when the entry is already in the protected segment? If it's there, just return it immediately. Then it's not necessary to run look it up in the probationary shard, right? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@595 PS12, Line 595: aren't nit: isn't ? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@596 PS12, Line 596: protected segment nit: the protected segment http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@596 PS12, Line 596: return lookup of entry in probationary segment return the entry found in the probationary segment http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@608 PS12, Line 608: vector<Cache::RLHandle*> evictions What if the entry that has just been inserted evicted right away with all the rest of the entries due to capacity limit? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@610 PS12, Line 610: protected segment nit: the protected segment http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@612 PS12, Line 612: for (size_t i = evictions.size(); i > 0; --i) { Could use reverse iterator instead of dancing with indices? -- 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: 12 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 14 Feb 2024 05:21:08 +0000 Gerrit-HasComments: Yes
