Dan Burkert has posted comments on this change. Change subject: KUDU-1812. Redact pretty-printed sensitive user data ......................................................................
Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/5555/4//COMMIT_MSG Commit Message: Line 53: > Should also add a note about what it means to use KUDU_REDACT in the C++ cl Done http://gerrit.cloudera.org:8080/#/c/5555/4/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: Line 519: // Ensure that redaction does not apply to partitions, since they are metadata. > Shouldn't this go above the CreatePartitions() call, for completeness? Done http://gerrit.cloudera.org:8080/#/c/5555/4/src/kudu/common/partition.h File src/kudu/common/partition.h: PS4, Line 204: // Returns a text description of this partition schema suitable for display in the web UI. : // The format of this string is not guaranteed to be identical cross-version. : // : // 'range_partitions' should include the set of range partitions in the table, : // as formatted by 'RangePartitionDebugString'. : std::string DisplayString(const Schema& schema, : const std::vector<std::string>& range_partitions) const; : : // Returns header and entry HTML cells for the partition schema for the master : // table web UI. This is an abstraction leak, but it's better than leaking the : // internals of partitions to the master path handlers. : std::string PartitionTableHeader(const Schema& schema) const; : std::string PartitionTableEntry(const Schema& schema, const Partition& partition) const; > Should probably be annotated as being metadata and thus not redacting. DisplayString doesn't doesn't format sensitive data and it doesn't disable redaction, so I think that would be misleading documentation. For the HTML ones: done Line 250: // Returns a text description of the encoded range key suitable for debug printing. > Likewise, for consistency with DebugStringComponents (also private). These don't disable redaction, so I think that would be misleading. Line 301: void AppendRangeDebugStringComponentsOrMin(const KuduPartialRow& row, > Likewise. Same here, this does not disable redaction. http://gerrit.cloudera.org:8080/#/c/5555/4/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: Line 52: return Substitute("SPLIT_ROW $0", split_row->ToString()); > Isn't a split point is morally equivalent to a partition boundary? Done http://gerrit.cloudera.org:8080/#/c/5555/4/src/kudu/tools/tool_main.cc File src/kudu/tools/tool_main.cc: Line 227: FLAGS_log_redact_user_data = false; > Let's add a comment explaining this. Done http://gerrit.cloudera.org:8080/#/c/5555/4/src/kudu/util/flag_tags-test.cc File src/kudu/util/flag_tags-test.cc: PS4, Line 54: // Set to true via KuduTest, but explicitly unset here as this test deals : // with unsafe and experimental flags. > Should update this comment to make it more clear that KuduTest overrides th Done http://gerrit.cloudera.org:8080/#/c/5555/4/src/kudu/util/flags-test.cc File src/kudu/util/flags-test.cc: PS4, Line 44: // Set to true via KuduTest, but explicitly unset here as this test parses : // command line flags and fails if an unsafe or experimental flag is set. > Likewise. Done http://gerrit.cloudera.org:8080/#/c/5555/4/src/kudu/util/logging-test.cc File src/kudu/util/logging-test.cc: Line 195: }); > Nit: indentation? Done http://gerrit.cloudera.org:8080/#/c/5555/4/src/kudu/util/logging.h File src/kudu/util/logging.h: Line 154: extern const char* const kRedactionMessage; > This is duplicated from just before ScopedDisableRedaction. Done -- To view, visit http://gerrit.cloudera.org:8080/5555 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2b87a3065280116bb8af6f26f072dafdfd1ee077 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
