Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17974 )
Change subject: [encryption] KUDU-3331 Encrypt file system ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/17974/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17974/2//COMMIT_MSG@12 PS2, Line 12: ntegrates this encryption support into the project nit: does it make sense to mention that there is at least one TODO to properly implement detection of whether a file is encrypted/unencrypted in tool_action_pbc? http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/consensus/consensus_meta-test.cc File src/kudu/consensus/consensus_meta-test.cc: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/consensus/consensus_meta-test.cc@82 PS2, Line 82: SetEncryptionFlags(bool encryption_enabled) nit: why not just 'EnableEncryption(bool enable)' ? http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/fs/log_block_manager-test.cc@229 PS2, Line 229: SetEncryptionFlags(bool enable) nit: why not just 'EnableEncryption(bool enable)' ? http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/fs/log_block_manager.cc@3005 PS2, Line 3005: RWFileOptions opts; : string tmp_file_name; : RETURN_NOT_OK_LBM_DISK_FAILURE_PREPEND(env_->NewTempRWFile(opts, tmpl, : &tmp_file_name, &tmp_file), Looked at this a bit, but could not understand why this change was necessary :) Could you please clarify on this just for me, maybe in this gerrit thread? Maybe, something is missing here (like setting the 'encryped' field)? http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/integration-tests/raft_consensus-itest.cc@828 PS2, Line 828: FLAGS_encryption = true; nit (here and below): could you add a comment explaining why adding flags to the set of tserver flags is not enough, so setting FLAGS_encryption is necessary as well? http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/util/env.h File src/kudu/util/env.h: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/util/env.h@383 PS2, Line 383: IsEncryptionEnabled Do you see it ever be requiring some non-const stuff in the implementation? If no, maybe consider adding 'const' specifier for this method? http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/util/env.h@421 PS2, Line 421: true Ah, interesting: I'd expect that everything is kept non-encrypted by default for the sake of backwards compatibility. At least that way it's not necessary to update the setting in so many files. Any special idea behind setting this to 'true' by default? http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/util/env_posix.cc@203 PS2, Line 203: encryption nit: maybe, name this flag 'enable_encryption' ? http://gerrit.cloudera.org:8080/#/c/17974/2/src/kudu/util/env_posix.cc@1997 PS2, Line 1997: virtual nit: drop 'virtual' since there is 'override' already -- 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: 2 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 26 Oct 2021 23:52:04 +0000 Gerrit-HasComments: Yes
