Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18192 )
Change subject: KUDU-3368 Encrypt file keys with server keys ...................................................................... Patch Set 4: (7 comments) after a first pass it looks okay, but I'm not sure I completely understand the changes related to the envs, probably have to do a second pass, apart from that, a few small nits http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/fs/fs.proto File src/kudu/fs/fs.proto: http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/fs/fs.proto@37 PS4, Line 37: nit: could you add a comment to describe what is this for? http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/fs/fs_manager.cc@130 PS4, Line 130: DECLARE_bool(encrypt_data_at_rest); nit: should these have some descriptions? http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/integration-tests/tablet_copy-itest.cc File src/kudu/integration-tests/tablet_copy-itest.cc: http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/integration-tests/tablet_copy-itest.cc@991 PS4, Line 991: NO_FATALS(StartClusterWithOpts(opts)); sorry if I'm missing something, but how can we know that the cluster was started with encryption enabled? http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/mini-cluster/external_mini_cluster.h@408 PS4, Line 408: virtual Env* ts_env(int ts_idx) const override; is the ts_env and master_env only used when env is not used? or is it possible to use all three at the same time? http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/tools/kudu-tool-test.cc@292 PS4, Line 292: LOG(INFO) << arg_str; is this needed, or was just left in after some debugging? :) http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/tools/tool_action_common.cc@910 PS4, Line 910: "Couldt not open instance file"); nit: typo, extra t in Could http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/util/test_util.h File src/kudu/util/test_util.h: http://gerrit.cloudera.org:8080/#/c/18192/4/src/kudu/util/test_util.h@30 PS4, Line 30: #include <glog/logging.h> is this needed? -- To view, visit http://gerrit.cloudera.org:8080/18192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1884f62fde3bb110291b1d01c5e68942c6071318 Gerrit-Change-Number: 18192 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: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 16 May 2022 17:35:09 +0000 Gerrit-HasComments: Yes
