Yan-Daojiang 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 2: (13 comments) Thanks again everyone for the detailed review. All comments have been addressed. I've also added benchmark tests, with the results summarized in the commit message. http://gerrit.cloudera.org:8080/#/c/24387/1//COMMIT_MSG Commit Message: PS1: > Did you happen to capture results reported by LogBlockManagerNativeMetaTest Benchmark tests were performed similarly to those at https://gerrit.cloudera.org/c/24326/. Results attached on KUDU-3779 [1]; summary table also in the updated commit description. [1] https://issues.apache.org/jira/browse/KUDU-3779 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@1413 PS1, Line 1413: // historically exercised the streaming path only. > Nice coverage. Done. The trailing-zero loop now sets FLAGS_v=1 and registers a StringVectorSink around each post-recovery reopen, then asserts that the VLOG(1) "Preloaded metadata file ..." line from MaybePreloadMetadataIntoMemory() was emitted at least once per tail_bytes case. A silent regression to the streaming path will trip this assertion. http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager-test.cc@1446 PS1, Line 1446: << "), but no preload log line was emitted"; : : // The reopen should > nit: could this be reduced to Done http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.h@227 PS1, Line 227: TestInMemoryReplayRecoversTrailingZeros); > nit: this should be above line:226 for order purpose. Done 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: against the > nit/question: I'm trying to understand what 'symmetrically' means here. Do Reworded. The new description spells out that the threshold is compared against the cleartext payload size in both cases, so the same flag value applies uniformly to encrypted and non-encrypted clusters. http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.cc@1486 PS1, Line 1486: ws below, so once we know the fi > nit: does it make sense to move this part before the 'for' cycle since we k Done http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.cc@1487 PS1, Line 1487: // the encryption header all subsequent slices are too. : if (PREDICT_FALSE(offset < encryption_header_size_)) { : return Status::IOError( > s/hits/hints/ Done http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.cc@1503 PS1, Line 1503: } > This relies on a single vectored Decrypt() over contiguous slices behaving Done. Added the constraint to RandomAccessFile::Decrypt()'s contract: a single vectored decrypt is only equivalent to per-slice decrypt when no *interior* slice is all-zero ciphertext (encrypted impls may short-circuit all-zero slices to preserve KUDU-2260 trailing-zero recovery and therefore not advance the EVP keystream). Callers needing per-slice semantics must invoke Decrypt() once per slice. Also cross-referenced the contract from MemoryReadableFile::ReadV. http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.cc@1545 PS1, Line 1545: > +1 for clarifying the comment Done. http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.cc@1614 PS1, Line 1614: // headers. > For the non-encrypted path this now keeps the underlying metadata file hand Done. MemoryReadableFile::source_ is now optional: it is only retained when header_size > 0. On the unencrypted path raw_reader goes out of scope right after the buffer is slurped and its fd is released immediately, restoring the pre-KUDU-3779 fd lifetime. http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/util/env.h File src/kudu/util/env.h: http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/util/env.h@519 PS1, Line 519: DCHECK_EQ(0, GetEncryptionHeader > nit: not important - see if you can add assert to ensure default implementa Done http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/util/env.h@533 PS1, Line 533: // slice is all-zero ciphertext. Encrypted implementations may short-circuit > The doc says slices are "assumed to be contiguous" and the env-test asserts Done http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/util/env_posix.cc@1082 PS1, Line 1082: DCHECK_GE(logical_offset, GetEncryptionHeaderSize()); > DCHECK is compiled out in release. The MemoryReadableFile::Read/ReadV bound Confirmed. In production RandomAccessFile::Decrypt() is reached from only two kinds of callers: (a) MemoryReadableFile::Read/ReadV, both of which reject offset < encryption_header_size_ up front via a release-build bounds check; (b) the pass-through Decrypt() overrides in file_cache.cc's wrapper and MemoryReadableFile itself, where the caller is bound by RandomAccessFile::Decrypt()'s public contract (offset >= GetEncryptionHeaderSize()). There is no release-build path that can feed logical_offset < header into PosixRandomAccessFile::Decrypt() — reaching it that way would be API misuse. -- 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: 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 Chovan <[email protected]> Gerrit-Comment-Date: Sun, 14 Jun 2026 14:04:38 +0000 Gerrit-HasComments: Yes
