Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21390 )
Change subject: KUDU-613: Integrate SLRU cache into block cache ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc File src/kudu/cfile/block_cache.cc: http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@235 PS2, Line 235: BlockCache::BlockCache() : : BlockCache(FLAGS_block_cache_capacity_mb * 1024 * 1024) { : } Interesting, but why this constructor isn't updated to take care of different type of the underlying cache? That's especially strange given the funny code below for the next constructor when the 'capacity' parameter is ignored upon using the SLRU type of the cache. http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@242 PS2, Line 242: CreateCache(FLAGS_block_cache_probationary_segment_capacity_mb * 1024 * 1024, : FLAGS_block_cache_protected_segment_capacity_mb * 1024 * 1024, : FLAGS_block_cache_lookups_before_upgrade)) { That's a bad smell -- how 'capacity' is then used in this branch? http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@247 PS2, Line 247: BlockCache::BlockCache(size_t probationary_segment_capacity, : size_t protected_segment_capacity, : size_t lookups) : : cache_(CreateCache(probationary_segment_capacity, protected_segment_capacity, lookups)) { : } With this code and the code above it smells even worse -- now there is a complete mess with trying to understand when a particular type of cache is created based on the constructor invoked and the --block_cache_eviction_policy setting. -- To view, visit http://gerrit.cloudera.org:8080/21390 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04411ab2756045f15a272f3397d46d871b087b03 Gerrit-Change-Number: 21390 Gerrit-PatchSet: 2 Gerrit-Owner: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Comment-Date: Fri, 10 May 2024 18:29:02 +0000 Gerrit-HasComments: Yes
