Todd Lipcon has posted comments on this change. Change subject: WIP: KUDU-1812. redaction of PB fields ......................................................................
Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/5553/2/src/kudu/util/pb_util.cc File src/kudu/util/pb_util.cc: Line 551: virtual string PrintFieldName(const Message& message, > Don't need virtual and override, right? Done PS2, Line 552: const Reflection* reflection, : const FieldDescriptor* field) const override { > Nit: indentation. Done Line 560: if (hide_next_string_) { > Shouldn't we reset hide_next_string_ to false after redacting? Or does it n Yea, it doesn't make a difference because we always print a new field name before printing some other field, but I'll reset anyway because it looks more natural. Line 593: if (debug_string.size() > 0 && > warning: the 'empty' method should be used to check for emptiness instead o Done http://gerrit.cloudera.org:8080/#/c/5553/2/src/kudu/util/pb_util.h File src/kudu/util/pb_util.h: Line 102: std::string SecureDebugString(const google::protobuf::Message& message); > warning: function 'kudu::pb_util::SecureDebugString' has a definition with Done Line 103: std::string SecureShortDebugString(const google::protobuf::Message& message); > warning: function 'kudu::pb_util::SecureShortDebugString' has a definition Done http://gerrit.cloudera.org:8080/#/c/5553/2/src/kudu/util/pb_util.proto File src/kudu/util/pb_util.proto: Line 43: optional bool REDACT = 50001 [default=false]; > I don't think changing the field # would be a breaking change, since this i I think although it's not "in-house", it's also not a "public application" in the sense that others would be directly interfacing with our protobuf files from other applications. I think this warning is mostly meant to discourage people implementing language bindings from assigning some special option like 'erlang_generator_do_foo' to an extension number that application programmers might want to use, etc. Furthermore, I agree that since it's not serialized in any way, we can always change it later. http://gerrit.cloudera.org:8080/#/c/5553/2/src/kudu/util/proto_container_test.proto File src/kudu/util/proto_container_test.proto: Line 29: // TODO move me to another file > Nit: update this? Done -- To view, visit http://gerrit.cloudera.org:8080/5553 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie07bfdcbc33d38d55315faae2c4906899a5450fb Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
