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 2: (7 comments) Thanks for the review. All issues have been addressed. Also added a new set of benchmarks and updated the commit message accordingly. Please take another look when you have time. 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 two preadv() syscalls > The description of this changelist and the code below mentions 2 pread() c Confirmed that two of "preadv" are accurate. http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@1452 PS1, Line 1452: result.size()' are both bounded above > Would result.size() + buf_off > data_.size() be more safe way to check for Done http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@1513 PS1, Line 1513: > How high is the probability of hitting non-OK status here and what might be The probability of a non-OK status is expected to be extremely low. I have followed the suggestion and marked the !s.ok() branch with PREDICT_FALSE to optimize branch prediction. http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@1520 PS1, Line 1520: // optimization inside DoDecryptV that skips decryption of all-zero : // ciphertext slices, > Shouldn't we first check for this condition before checking for the file si Done, one syscall can be omitted for encrypted paths. http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@1536 PS1, Line 1536: KLOG_EVERY_N_SECS(WARNING, 10) : << "Failed to stat metadata file " << metadata_path : << " for in-memory replay; falling back to streaming reads: " : << s.ToString(); : > Is this even relevant given there is a check if (header_size > 0) above? I Good point — the current comment was indeed misleading in this context. I’ve refactored the logic to make it clearer and consolidated the handling into the file_size == 0 case, since it already implicitly covers this scenario. http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@1549 PS1, Line 1549: > Does it make sense to wrap this into PREDICT_FALSE? Done, same as above http://gerrit.cloudera.org:8080/#/c/24326/1/src/kudu/fs/log_block_manager.cc@1558 PS1, Line 1558: << "falling back to streaming reads: " << s.ToString(); : return nullptr; > Wouldn't std::unique_ptr be enough here? Why to introduce shared ownership You’re right — std::unique_ptr is sufficient here since there is no need for shared ownership. I’ve updated the implementation to use unique_ptr<MemoryReadableFile> accordingly. -- 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: Mon, 25 May 2026 15:47:53 +0000 Gerrit-HasComments: Yes
