Todd Lipcon has posted comments on this change. Change subject: KUDU-1812: redact row contents from logs ......................................................................
Patch Set 2: (5 comments) this looks pretty reasonable, but it's very hard to review for completeness. As we discussed on Slack, it would be nicer if the default behavior were to redact everything and we explicitly tagged the places where we stringified user data that we _didn't_ want redacted. Let me do a little prototyping and see what I can come up with. http://gerrit.cloudera.org:8080/#/c/5555/2/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: Line 524: VLOG(3) << "Looking up tablet for " << KUDU_REDACT(op->write_op->ToString()); isn't this ToString() already safe? Line 574: << "Could not remove op " << KUDU_REDACT(op->ToString()) << " from in-flight list"; same (and several below) http://gerrit.cloudera.org:8080/#/c/5555/2/src/kudu/client/client.h File src/kudu/client/client.h: Line 2115 hrm, was this ToString never implemented? http://gerrit.cloudera.org:8080/#/c/5555/2/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: Line 466: VLOG(1) << "Pushing down predicate " << KUDU_REDACT(pred.ToString()); would be nice to keep the column name, operator, etc here http://gerrit.cloudera.org:8080/#/c/5555/2/src/kudu/util/logging.h File src/kudu/util/logging.h: Line 180: // as a macro so that the message can be lazily evaluated. I think lazy evaluation is good in the context of a release build, but I'm afraid we're not going to get great coverage of the non-redacted code path. In debug builds, maybe we should make this be a non-lazy function so that, if for example we tried to stringify a NULL pointer, we'd still get a crash in our DEBUG/ASAN test cases? Or do you think the fact that we've defauled log-redaction to off in KuduTest is sufficient? -- 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: 2 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
