Adar Dembo has posted comments on this change. Change subject: WIP: KUDU-1812. redaction of PB fields ......................................................................
Patch Set 2: (6 comments) I did a pass over the .proto files and I think you've nailed everything. The most important is obviously RowOperationsPB, and I hope we do a good job of using it everywhere for row data. We do at least for writes and for consensus replication. http://gerrit.cloudera.org:8080/#/c/5553/2//COMMIT_MSG Commit Message: Line 11: - perhaps implement hashing/encryption instead of just redaction? Out of scope for this round of changes, I think. 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? PS2, Line 552: const Reflection* reflection, : const FieldDescriptor* field) const override { Nit: indentation. Line 560: if (hide_next_string_) { Shouldn't we reset hide_next_string_ to false after redacting? Or does it not matter, because it's not possible for there to be two consecutive PrintString/PrintBytes without a PrintFieldName call in between? 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]; The protobuf docs say: Since custom options are extensions, they must be assigned field numbers like any other field or extension. In the examples above, we have used field numbers in the range 50000-99999. This range is reserved for internal use within individual organizations, so you can use numbers in this range freely for in-house applications. If you intend to use custom options in public applications, however, then it is important that you make sure that your field numbers are globally unique. To obtain globally unique field numbers, please send a request to protobuf-global-extension-regis...@google.com. Simply provide your project name (e.g. Object-C plugin) and your project website (if available). Kudu isn't an "in-house application", so does that mean we need to request a globally unique field number? 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? -- 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-HasComments: Yes
