Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24387 )
Change subject: KUDU-3779: support in-memory metadata replay for encrypted clusters ...................................................................... Patch Set 1: Code-Review+1 (6 comments) Thank you for implementing this optimization for the encrypted file case! Overall this looks good to me after a quick look. A few nits in addition to Marton's feedbackon PS1. http://gerrit.cloudera.org:8080/#/c/24387/1//COMMIT_MSG Commit Message: PS1: Did you happen to capture results reported by LogBlockManagerNativeMetaTest.InMemoryReplayStartupBenchmark to compare run-time with and without this feature enabled? If yes, feel free to add the information into the commit description. If not, there isn't a pressure to run it. http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager-test.cc@1446 PS1, Line 1446: ASSERT_GE(good_meta_size, header); : const uint64_t payload = good_meta_size - header; : ASSERT_GT(payload, 0); nit: could this be reduced to ASSERT_GT(good_meta_size, header); const uint64_t payload = good_meta_size - header; ? http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.cc@181 PS1, Line 181: symmetrically nit/question: I'm trying to understand what 'symmetrically' means here. Does it refer to the approximately similar size of the data for encrypted and non-encrypted case, or that's something else? http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.cc@1486 PS1, Line 1486: offset < encryption_header_size_ nit: does it make sense to move this part before the 'for' cycle since we know that offset can only increase each next iteration, and encryption_header_size_ is a constant? http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.cc@1487 PS1, Line 1487: s.size() > data_.size() || : offset - encryption_header_size_ > : data_.size() - s.size()) IIUC, we could drop the 's.size() > data_.size()' condition since the second condition is stronger (just moving around parts to switch to the absolute offsets): s.size() + offset > data_.size() + encryption_header_size_ Since we know that offset >= encryption_header_size_ always, right? I.e., if 's.size() > data_.size()' is true, then 's.size() + offset > data_.size() + encryption_header_size_' is true as well. But if the latter is true, the former might not yet be true. Most likely the compiler already optimizes this, but it's a bit easier to comprehend with less symbols and comparisons. Consider adding corresponding comments, if you want to provide the hits on the logic reduction. http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.cc@1545 PS1, Line 1545: Never read from / written to after construction. > Maybe I'm missing something, but the comment "Never read from / written to +1 for clarifying the comment -- To view, visit http://gerrit.cloudera.org:8080/24387 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I37af5bc37613dd4081e1ad708f084e1c88fb3e75 Gerrit-Change-Number: 24387 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 Chovan <[email protected]> Gerrit-Comment-Date: Tue, 09 Jun 2026 07:45:40 +0000 Gerrit-HasComments: Yes
