Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [encryption] KUDU-3316 Add encrypted file keys to files ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/18025/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18025/3//COMMIT_MSG@30 PS3, Line 30: File sizes as returned by *File APIs are logical, so on disk they're 40 : bytes larger. The GetFileSize() can return both the logical and the : physical file size, but GetFileSizeOnDisk() actually uses block count : and must be physical. Some of the tests rely on this and make : assumptions based on the numbers reported here, these tests are not run : with encryption enabled. I'm curious what the policy is for using physical vs logical size then. I would have thought we'd want to use physical size everywhere we possibly can, since it's as accurate for an end-user with regards to what is using space. Though it seems like that's not the case for some files? eg consensus and tablet metadata? http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/consensus/consensus_meta.cc File src/kudu/consensus/consensus_meta.cc: http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/consensus/consensus_meta.cc@416 PS3, Line 416: false Shouldn't this show the header too? 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@1431 PS3, Line 1431: EncryptionHeader Should be a const ref to avoid a copy http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1432 PS3, Line 1432: "kuduenc" nit: consider defining this once. Also extra semicolon http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1460 PS3, Line 1460: uint8_t zeros[16] = {0}; Could be const and static? http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1461 PS3, Line 1461: zeross nit: IMO this reads like a typo. Consider zero_slice or zero_slc or something? http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1582 PS3, Line 1582: ;; nit: extra semicolon http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1710 PS3, Line 1710: exclude_encryption_header nit: consider some WITH_HEADER enum? http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@2256 PS3, Line 2256: RETURN_NOT_OK(GenerateHeader(&eh)); : RETURN_NOT_OK(WriteEncryptionHeader(fd, eh)); Is there any concern that a user accidentally switches between encryption and no encryption, and overwrites some data in this way? I guess not, since presumably we'd fail to start up if the instance files differ in encryption status before doing any real damage, but still worth thinking about. -- 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: 3 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 16 Nov 2021 21:10:24 +0000 Gerrit-HasComments: Yes
