Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [encryption] KUDU-3316 Add encrypted file keys ...................................................................... Patch Set 4: (10 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? I actually ended up using 64 bytes after all. It's not needed for alignments as specified in the design doc, but it's still good to have some bytes reserved for future use in case we want too add support for IV or other algorithms using larger keys in the future. Also, it's easier to see where the header ends and the data starts when looking at a hexdump :) 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 Done 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 what's wrong with the spacing here? 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? Yes, but tablet_block_size is the size of only a block, session_block_size is the whole file size of a single-block temp file. Changed it to account for the difference instead of removing the assertion. 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 cer Done 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? Done 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 fil This was part of another way I tried to write headers (during the first append to a file instead of when opening), but I ended up not using this. Removed it, thanks for bringing my attention to it. 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 Yea, this is not actually needed now (I needed the max of 3 integers when I was trying to hide the true file size). 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 g Yes, you're right, thanks for bringing this up, left this in by mistake. 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? Done -- 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 10:08:55 +0000 Gerrit-HasComments: Yes
