Yan-Daojiang 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 3: (7 comments) Thanks for all the feedback! I’ve tried to address all the issues that were pointed out. The fixes should theoretically not affect the test conclusions mentioned in the commit message. I’d appreciate it if you could take another look when you have time. 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 Done 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: ASSERT_OK(deletion_transaction->CommitDeletedBlocks(nullptr)); : } : } > The check is happening post AddDeletedBlock. So, if to_delete_count_per_bat Done. Also fixed this issue in StartupBenchmark http://gerrit.cloudera.org:8080/#/c/24326/2/src/kudu/fs/log_block_manager-test.cc@1310 PS2, Line 1310: is->bm_->OpenBlock > nit: Maybe open a block just for sanity check after opening BM - if it is n Done http://gerrit.cloudera.org:8080/#/c/24326/2/src/kudu/fs/log_block_manager-test.cc@2089 PS2, Line 2089: > Does it make sense to verify that blocks that were supposed to get deleted Done 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: OError > With 'offset' (line:1553) and 'encryption_header_size_' (lines:1563-1564) a Good catch. The encryption_header_size_ machinery is left-over scaffolding from an earlier attempt to make this fast path also support encrypted metadata. The original plan was to slurp the whole file (encryption header + ciphertext) into memory, and make MemoryReadableFile mimic PosixRandomAccessFile's "logical offset includes the header" semantics. However, I ran into a correctness issue back then which could not be resolved cleanly without touching the encryption layer. I've removed the encryption_header_size_ member variable, its corresponding constructor parameter, the offset-translation branch in Read(), and related logic in Size() and GetEncryptionHeaderSize(). I also updated the class header to explicitly state that MemoryReadableFile only works with plaintext data, and referenced the comment in MaybePreloadMetadataIntoMemory to explain why encrypted files do not take this fast path. If we revisit encrypted file support in the future, we could reimplement the header-aware logic based on a clean design. http://gerrit.cloudera.org:8080/#/c/24326/2/src/kudu/fs/log_block_manager.cc@1554 PS2, Line 1554: atus LogBlockContainerNativeMeta::ProcessRecords( : FsReport* report, : LogBlockManager::UntrackedBlockMap* live_blocks, : LogBlockManager::BlockRecordMap* live_block_records, : vector<LogBlockRefPtr>* dead_blocks, : uint64_t* max_block_id, : > Does it make sense to add a unit test to validate that failure cases bring I think the existing TestInMemoryMetadataReplay case 2 (threshold_bytes = 1) already coverd the streaming fallback path: when MaybePreloadMetadataIntoMemory returns nullptr for any reason, ProcessRecords` moves raw_reader into the ReadablePBContainerFile and we end up on the exact old streaming code path. http://gerrit.cloudera.org:8080/#/c/24326/2/src/kudu/fs/log_block_manager.cc@1590 PS2, Line 1590: ssRecord(& > Would it make sense to release 'raw_reader' (in else condition) if data has Done -- 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: 3 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 10:25:41 +0000 Gerrit-HasComments: Yes
