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 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/fs/fs_manager.h@207 PS5, Line 207: out( > nit: it would be nice to document the behavior of this method to reflect th Done http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/fs/fs_manager.h@292 PS5, Line 292: const std::string& uuid() const; > Would be nice to document the essence of this new method. What does it ret Done http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/mini-cluster/internal_mini_cluster.h File src/kudu/mini-cluster/internal_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/mini-cluster/internal_mini_cluster.h@167 PS5, Line 167: // Returns the default environment. As the servers in an internal mini-cluster > nit: maybe, add a doc blurb to explain the essence of why the same default Done http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/tools/tool_action_fs.cc@88 PS5, Line 88: " > nit: remove the extra space DER/PEM are only for public key crypto AFAIK, and this is simply a symmetric key with no certificates attached. This just expects the raw key in hexadecimal format. http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/util/env.h File src/kudu/util/env.h: http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/util/env.h@80 PS5, Line 80: Unlike the default env, this is not owned by Kudu, and : // must be destroyed when not used anymore > With this, maybe change the signature of this method to return std::unique_ Good point, thanks for the tip. http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/util/env_util.cc File src/kudu/util/env_util.cc: http://gerrit.cloudera.org:8080/#/c/18192/5/src/kudu/util/env_util.cc@243 PS5, Line 243: SequentialFileOptions source_opts; : source_opts.is_sensitive = opts.is_sensitive; : RETURN_NOT_OK(env->NewSequentialFile(source_opts, source_path, &source)); : uint64_t size; > Is this comment still valid after change at line 248? Thanks, I missed this. -- 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: 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: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 23 May 2022 18:51:02 +0000 Gerrit-HasComments: Yes
