Marton Greber 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: (6 comments) A couple questions/nits, else it does look good to me. Thanks for working on this! 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: for (const auto tail_bytes : {1, 8, 128, 4096}) { Nice coverage. nit: Could we also assert the post-recovery reopen still takes the in-memory path so a silent regression to streaming wouldn't pass this test unnoticed? 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@1503 PS1, Line 1503: return source_->Decrypt(orig_offset, results); This relies on a single vectored Decrypt() over contiguous slices behaving identically to decrypting each slice individually. That's true for non-zero slices, but DoDecryptV's per-slice IsAllZeros() `continue` does not advance the EVP keystream, so an *interior* all-zero ciphertext slice followed by a non-zero slice would mis-decrypt the later slice. pb_util only ever issues single-slice Read()s here so this path isn't currently exercised, and PosixRandomAccessFile::ReadV already had this property, so it's not a regression - but since Decrypt() is now a documented public virtual, should the contract note that vectored decrypt is only equivalent to per-slice when no interior slice is all-zeros? 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 after construction" is a little misleading since Decrypt() does read source_'s encryption header on every in-memory Read(). Suggest clarifying it means "no further disk I/O is performed against it". Can you please clarify on this? http://gerrit.cloudera.org:8080/#/c/24387/1/src/kudu/fs/log_block_manager.cc@1614 PS1, Line 1614: shared_ptr<RandomAccessFile> source(std::move(raw_reader)); For the non-encrypted path this now keeps the underlying metadata file handle (a real fd from env->NewRandomAccessFile, not file-cache-managed) open for the entire ProcessRecords() replay, whereas the pre-KUDU-3779 in-memory path closed it as soon as the buffer was slurped. Decrypt() is a no-op when header_size == 0, so source_ is never actually needed in that case. With many containers replaying concurrently this raises peak open-fd count vs. the previous in-memory behavior (it's no worse than the streaming path, but it does give back one of that optimization's side benefits). Worth only storing source_ when header_size > 0 and treating a null source_ as "Decrypt is a no-op"? 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@533 PS1, Line 533: virtual Status Decrypt(uint64_t /*offset*/, The doc says slices are "assumed to be contiguous" and the env-test asserts vectored decrypt "must behave the same as decrypting each slice individually" - that equivalence only holds for non-all-zero slices (see DoDecryptV's keystream skip). Consider documenting that caveat here so future callers don't assume general vector/per-slice equivalence. 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 bounds checks already reject offset < header before reaching here, and pb_util never decrypts inside the header, so this is reachable only via misuse. Fine to leave as DCHECK, just confirming there's no release-build path that could pass logical_offset < header (which would produce a bogus IV rather than an error). -- 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: Wed, 03 Jun 2026 13:52:52 +0000 Gerrit-HasComments: Yes
