Attila Bukor 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) Thanks for the review, addressed your comments. As for the env part, we can have a quick chat about it. 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? Done 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? These are only declared here so that they can be used in this file, they have descriptions where they're defined. 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 st I set encryption flags to true in line 988. 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 possi All of these return an Env*. If the cluster is encrypted, the server key returned by env() is incorrect. Added a comment about this. 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? :) Done 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 Done 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? Done -- 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: Tue, 17 May 2022 11:25:31 +0000 Gerrit-HasComments: Yes
