Alexey Serbin 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 1: (7 comments) Overall looks good; a few nits and questions. Please also include the perf results that you posted as a response to gerrit comments into the git changelist description, at least the top-level summary. Thanks a lot! http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@173 PS1, Line 173: issuing one pread() per record The description of this changelist and the code below mentions 2 pread() calls per record. Which one is accurate? http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@1452 PS1, Line 1452: result.size() > data_.size() - buf_off Would result.size() + buf_off > data_.size() be more safe way to check for this condition? http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@1513 PS1, Line 1513: s.ok() How high is the probability of hitting non-OK status here and what might be the reason behind non-OK result status? Should we wrap !s.ok() into PREDICT_FALSE? http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@1520 PS1, Line 1520: const size_t header_size = raw_reader->GetEncryptionHeaderSize(); : if (header_size > 0) { Shouldn't we first check for this condition before checking for the file size? Assuming an error in getting the file size is of low probability, swapping the order could save a few cycles when working with encrypted blocks, no? http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@1536 PS1, Line 1536: if (file_size <= header_size) { : // No payload past the encryption header; nothing to preload. The streaming : // path will detect EOF immediately. : return nullptr; : } Is this even relevant given there is a check if (header_size > 0) above? In any case, the comment looks a bit misleading in this context. http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@1549 PS1, Line 1549: !s.ok() Does it make sense to wrap this into PREDICT_FALSE? http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@1558 PS1, Line 1558: return std::make_shared<MemoryReadableFile>( : metadata_path, std::move(buf), header_size); Wouldn't std::unique_ptr be enough here? Why to introduce shared ownership for MemoryReadableFile instance? -- 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: 1 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: Fri, 22 May 2026 18:49:31 +0000 Gerrit-HasComments: Yes
