Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22478 )
Change subject: KUDU-613: Fix BlockCache Constructor ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/22478/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22478/1//COMMIT_MSG@11 PS1, Line 11: policy. This patch fixes this miscalculation. : > I was thinking about adding an ASSERT statement after calculating the capac Could you add a trivial check for the expected sizes of each of the SLRU cache shards based on 'ideal' values for FLAGS_block_cache_protected_segment_percentage and FLAGS_block_cache_capacity_mb where no rounding errors are present? Such a test should have caught the bug if it was present. It shouldn't a big deal given we already have some scaffolding in block_cache-test.cc and you have spent months observing strange results while evaluating the performance of the SLRU cache, should it? http://gerrit.cloudera.org:8080/#/c/22478/1/src/kudu/cfile/block_cache.cc File src/kudu/cfile/block_cache.cc: http://gerrit.cloudera.org:8080/#/c/22478/1/src/kudu/cfile/block_cache.cc@253 PS1, Line 253: static_cast<int6 > Good point, casted it to a int64. As for the loss of precision for large do I don't think precision loss is something we should be concerned about: FLAGS_block_cache_capacity_mb's default is 512 and in real world cluster it's usually set to a few GiBytes at least. If you think that's a problem, I can suggest the following alternatives: * compute the size of the protected segment based numbers rounding of block_cache_protected_segment_percentage multiplication, but compute the size of the probation segment by subtracting the former integer number from FLAGS_block_cache_capacity_mb * 1024 * 1024 * consider changing block_cache_protected_segment_percentage to an integer flag, and adopt semantics that's similar to memory_pressure_percentage and other existing flags like that My comment on the float-to-integer conversions was to warn on corner cases where values might be close to INT64_MAX, but that's not applicable in this particular case since we are working with realistic amount of available memory on a machine. http://gerrit.cloudera.org:8080/#/c/22478/2/src/kudu/cfile/block_cache.cc File src/kudu/cfile/block_cache.cc: http://gerrit.cloudera.org:8080/#/c/22478/2/src/kudu/cfile/block_cache.cc@248 PS2, Line 248: FLAGS_block_cache_capacity_mb * 1024 * 1024 style nit: does it make sense to introduce a named constant for this and reuse it in this constructor here and below? -- To view, visit http://gerrit.cloudera.org:8080/22478 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icfde56fd766ba7160052e88ca09a63845f3297c6 Gerrit-Change-Number: 22478 Gerrit-PatchSet: 2 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Wed, 12 Feb 2025 21:09:22 +0000 Gerrit-HasComments: Yes
