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: (12 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: made to code that handles files and relies on sizes and offsets, : including in tests. : : I ran the full test suite manually with encryption enabled to make sure : turning on encryption doesn't break anything. : > I'm curious what the policy is for using physical vs logical size then. I w You're right, it's better to use physical sizes everywhere and update the code to take the header size into account. 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: > Shouldn't this show the header too? Done http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env.h File src/kudu/util/env.h: http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env.h@194 PS3, Line 194: > Looking at proliferation of extra parameters for the methods like this, I s Actually, that was my original approach, but decided against it, I don't remember exactly why, but I think this was clearer. Also, these are now gone with the simplification of the size calculation, so it shouldn't be a problem. http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env.h@730 PS3, Line 730: WriteStringToFileSync(Env* env > nit: maybe, name this WriteStringToFileEncryptedSync()? It seems the namin Done 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@919 PS3, Line 919: return write_fd_; > Consider adding DCHECK()/CHECK() on st.st_size being greater or equal to kE Done http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1431 PS3, Line 1431: > Should be a const ref to avoid a copy Done http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1432 PS3, Line 1432: > nit: consider defining this once. Done http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1460 PS3, Line 1460: } > Could be const and static? Done http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1461 PS3, Line 1461: > +1 nit, maybe call it padding? Done http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1582 PS3, Line 1582: > nit: extra semicolon Done http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@1710 PS3, Line 1710: > nit: consider some WITH_HEADER enum? Done http://gerrit.cloudera.org:8080/#/c/18025/3/src/kudu/util/env_posix.cc@2256 PS3, Line 2256: return IOError("stat", errno); : } > Is there any concern that a user accidentally switches between encryption a Yea, startup should/will fail in this case. Do you have any tips on how to make this safer? -- 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: Thu, 06 Jan 2022 20:03:08 +0000 Gerrit-HasComments: Yes
