Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17974 )
Change subject: [encryption] KUDU-3331 Encrypt file system ...................................................................... Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/17974/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17974/7//COMMIT_MSG@32 PS7, Line 32: needs nit: seems to be an extra word? http://gerrit.cloudera.org:8080/#/c/17974/7//COMMIT_MSG@45 PS7, Line 45: ronment set-up. Just to double-check: the results below are for RELEASE builds, right? http://gerrit.cloudera.org:8080/#/c/17974/6/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/17974/6/src/kudu/tserver/tablet_copy_client.cc@492 PS6, Line 492: WritableFileOptions() What about explicitly controlling the encryption for the result file here? http://gerrit.cloudera.org:8080/#/c/17974/6/src/kudu/tserver/tablet_copy_client.cc@798 PS6, Line 798: WritableFileOptions What about explicitly controlling the encryption for the result file here? http://gerrit.cloudera.org:8080/#/c/17974/7/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/17974/7/src/kudu/util/env_posix.cc@203 PS7, Line 203: Whether file system should be encrypted. nit: since now the terminology is more stable (encrypted vs sensitive, etc.), maybe update this description to be a bit more specific, i.e. Whether to encrypt sensitive files http://gerrit.cloudera.org:8080/#/c/17974/7/src/kudu/util/env_posix.cc@1306 PS7, Line 1306: opts.is_sensitive && IsEncryptionEnabled() nit: would it make sense to introduce a helper function/method ShouldEncrypt(const SequentialFileOptions&) to use here and below? http://gerrit.cloudera.org:8080/#/c/17974/6/src/kudu/util/rolling_log.cc File src/kudu/util/rolling_log.cc: http://gerrit.cloudera.org:8080/#/c/17974/6/src/kudu/util/rolling_log.cc@163 PS6, Line 163: opts.is_sensitive = false; Just to make sure this assignment is left here intentionally even if 'is_sensitive' is 'false' by default? -- To view, visit http://gerrit.cloudera.org:8080/17974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I909d0c4af0c1fca0d14c99a6627842dbe2ed7524 Gerrit-Change-Number: 17974 Gerrit-PatchSet: 6 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: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 17 Nov 2021 01:00:26 +0000 Gerrit-HasComments: Yes
