Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [encryption] KUDU-3316 Add encrypted file keys ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/18025/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18025/4//COMMIT_MSG@29 PS4, Line 29: 64- 40-byte? http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/consensus/log.cc@552 PS4, Line 552: auto file = active_segment_->file(); : RETURN_NOT_OK(file->Truncate(active_segment_ Merge lines? This makes a copy http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/fs/log_block_manager.cc@749 PS4, Line 749: nit: spacing http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/tserver/tablet_copy_source_session-test.cc File src/kudu/tserver/tablet_copy_source_session-test.cc: http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/tserver/tablet_copy_source_session-test.cc@a346 PS4, Line 346: Why this change? Aren't they both either encrypted or not? http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env.h File src/kudu/util/env.h: http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env.h@378 PS4, Line 378: uint8_t nit: maybe use size_t? GSG suggests erring away from unsigned except in certain scenarios https://google.github.io/styleguide/cppguide.html#Integer_Types http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@2256 PS3, Line 2256: return IOError("stat", errno); : } > Yea, startup should/will fail in this case. Do you have any tips on how to Will ponder further, though I suppose it's unlikely a file will be <40 bytes http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@602 PS4, Line 602: // Uncomment? http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@1083 PS4, Line 1083: header_written_ Missing a ! ? Does that mean that we're getting double headers for temp files? http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@1104 PS4, Line 1104: {filesize_, pre_allocated_size_} Why this change? Not a huge change, but the initializer list does add some instructions and changes the called function https://godbolt.org/z/o6bjP3Wsc push rbp mov rbp, rsp sub rsp, 32 mov DWORD PTR [rbp-20], edi mov eax, DWORD PTR [rbp-20] add eax, 1 mov DWORD PTR [rbp-4], eax lea rdx, [rbp-4] lea rax, [rbp-20] mov rsi, rdx mov rdi, rax call int const& std::max<int>(int const&, int const&) mov eax, DWORD PTR [rax] leave ret vs push rbp mov rbp, rsp push rbx sub rsp, 40 mov DWORD PTR [rbp-36], edi mov ecx, DWORD PTR [rbp-36] mov DWORD PTR [rbp-24], ecx mov ecx, DWORD PTR [rbp-36] add ecx, 1 mov DWORD PTR [rbp-20], ecx lea rcx, [rbp-24] mov rax, rcx mov edx, 2 mov rcx, rax mov rbx, rdx mov rax, rdx mov rdi, rcx mov rsi, rax call int std::max<int>(std::initializer_list<int>) mov rbx, QWORD PTR [rbp-8] leave ret http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@1657 PS4, Line 1657: , true I may be missing something, but it seems with its current usage, we could get by removing this, and conditioning whether to write based on 'encrypt', since we seem to always be writing the header for encrypted files before creating the new file. Is that the case? http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/util/env_posix.cc@2323 PS4, Line 2323: if (file_size < kEncryptionHeaderSize && encrypt) { : RETURN_NOT_OK(GenerateHeader(&eh)); : RETURN_NOT_OK(WriteEncryptionHeader(fd, fname, eh)); : header_written = true; : file_size = kEncryptionHeaderSize; : } else if (encrypt) { : RETURN_NOT_OK(ReadEncryptionHeader(fd, fname, &eh)); : header_written = true; : } nit: maybe less error prone as done elsewhere? if (encrypt) { if (file_size < kEncryptionHeaderSize) { ... } else { ... } header_written = true; } -- To view, visit http://gerrit.cloudera.org:8080/18025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd Gerrit-Change-Number: 18025 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Fri, 07 Jan 2022 01:28:00 +0000 Gerrit-HasComments: Yes
