Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/24326 )
Change subject: [fs] speed up LBM startup by replaying small metadata files from memory ...................................................................... Patch Set 2: (7 comments) Thank you for working on this! Overall looks good to me. I just have some minor comments. http://gerrit.cloudera.org:8080/#/c/24326/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24326/2//COMMIT_MSG@19 PS2, Line 19: RandomAccessFil nit: RandomAccessFile http://gerrit.cloudera.org:8080/#/c/24326/2/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/24326/2/src/kudu/fs/log_block_manager-test.cc@1287 PS2, Line 1287: if (--to_delete_count_per_batch <= 0) { : break; : } The check is happening post AddDeletedBlock. So, if to_delete_count_per_batch is 0, the loop will still add one block to deleted_blocks_ list. It may not cause any functional issue but the accounting-wise it doesn't seem to align. Maybe add the to_delete_count_per_batch check condition in for loop itself? Same goes for line 1192-1194 above http://gerrit.cloudera.org:8080/#/c/24326/2/src/kudu/fs/log_block_manager-test.cc@1310 PS2, Line 1310: ReopenBlockManager nit: Maybe open a block just for sanity check after opening BM - if it is not already happening inside ReopenBlockManager. http://gerrit.cloudera.org:8080/#/c/24326/2/src/kudu/fs/log_block_manager-test.cc@2089 PS2, Line 2089: // deleted above Does it make sense to verify that blocks that were supposed to get deleted have actually been deleted from LBM perspective? Maybe assert that the block is not found while trying to open. http://gerrit.cloudera.org:8080/#/c/24326/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/24326/2/src/kudu/fs/log_block_manager.cc@1445 PS2, Line 1445: offset With 'offset' (line:1553) and 'encryption_header_size_' (lines:1563-1564) always zero, what is the use of this condition? Unless this class is supposed to handle encrypted data as well (as per definition of log_container_metadata_inmem_replay_threshold_bytes flag). http://gerrit.cloudera.org:8080/#/c/24326/2/src/kudu/fs/log_block_manager.cc@1554 PS2, Line 1554: if (PREDICT_FALSE(!s.ok())) { : KLOG_EVERY_N_SECS(WARNING, 10) : << "Failed to preload metadata file " << metadata_path : << " (" << file_size << " bytes) for in-memory replay; " : << "falling back to streaming reads: " << s.ToString(); : return nullptr; : } Does it make sense to add a unit test to validate that failure cases bring back the old behavior of streaming reads? http://gerrit.cloudera.org:8080/#/c/24326/2/src/kudu/fs/log_block_manager.cc@1590 PS2, Line 1590: raw_reader Would it make sense to release 'raw_reader' (in else condition) if data has been successfully read into memory, to avoid keeping it open for lifetime of ProcessRecords? -- To view, visit http://gerrit.cloudera.org:8080/24326 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacbe12977ae945e7fa2f97a41aef250b03495cd4 Gerrit-Change-Number: 24326 Gerrit-PatchSet: 2 Gerrit-Owner: Yan-Daojiang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Yan-Daojiang <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 26 May 2026 07:46:34 +0000 Gerrit-HasComments: Yes
