Dan Burkert has posted comments on this change. Change subject: KUDU-1812: redact sensitive partition keys from logs ......................................................................
Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/5548/1/src/kudu/client/meta_cache.h File src/kudu/client/meta_cache.h: PS1, Line 324: This string will not be redacted, since table partitions : // are considered metadata. > Nit: put this on a new line, like you did in partition.h. Done http://gerrit.cloudera.org:8080/#/c/5548/1/src/kudu/common/partition.cc File src/kudu/common/partition.cc: Line 325: } else if (lower == *split || upper == *split) { > warning: don't use else after return [readability-else-after-return] Done PS1, Line 628: <redacted> > May be nice to store "<redacted>" somewhere centrally so we can easily chan Done PS1, Line 685: RangeKeyDebugString(key, schema) > An extra parameter -- should be dropped. Done http://gerrit.cloudera.org:8080/#/c/5548/1/src/kudu/common/partition.h File src/kudu/common/partition.h: Line 128: class PartitionSchema { > Would be good to include a high level discussion of redaction as it pertain Done PS1, Line 197: std::string RangeKeyDebugString(Slice range_key, const Schema& schema) const; : std::string RangeKeyDebugString(const KuduPartialRow& key) const; : std::string RangeKeyDebugString(const ConstContiguousRow& key) const; > These can be private. Also, the warning may not be necessary in that case, Done PS1, Line 302: /// Returns the stringified hash and range schema componenets of the partition : /// schema. > Nit: the triple-slash notation here was probably a mistake; could you fix i Done http://gerrit.cloudera.org:8080/#/c/5548/1/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: PS1, Line 419: number of partition ranges being scanned > Why is this sensitive? I've updated the comment - the real reason is the start/end keys were leaking. http://gerrit.cloudera.org:8080/#/c/5548/1/src/kudu/util/logging.cc File src/kudu/util/logging.cc: Line 65: DEFINE_bool(log_row_contents, false, > I'd prefer if the gflag name were more generic, on two fronts: Going to leave this open until the next revision, I agree the name needs to be changed or at least thought about more. PS1, Line 66: log and error > nit: may be just 'log messages'? I'm not sure how to differentiate between Right now this flag is also causing sensitive data in error messages to be redacted. -- To view, visit http://gerrit.cloudera.org:8080/5548 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3eba5290145048d77d551b989865c05d928a9b10 Gerrit-PatchSet: 1 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
