Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/6755 )
Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help ...................................................................... Patch Set 6: (19 comments) I don't understand why we need per-context redaction macros if redaction is now controlled on a per context rather than per data basis. I would have expected more prolific use of ScopedDisableRedaction and/or KUDU_DISABLE_REDACTION instead. For example, if web UI redaction is disabled, I would expect a web UI thread to temporarily create a ScopedDisabledRedaction when processing _any_ user request, which would temporarily disable all instances of KUDU_REDACT in that thread. Disabling redaction for glogs is a little trickier though, since LOG messages are peppered everywhere. I'm curious to hear Todd or Dan's take on this. http://gerrit.cloudera.org:8080/#/c/6755/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/6755/6//COMMIT_MSG@15 PS6, Line 15: controls control http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/common/encoded_key.cc File src/kudu/common/encoded_key.cc: http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/common/encoded_key.cc@190 PS6, Line 190: string EncodedKey::RangeToString(const EncodedKey* lower, const EncodedKey* upper) { There's no guarantee that RangeToString has to be called only from the web UI. Maybe rename it RangeToStringForWebUI to make that more clear? http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/server/default-path-handlers.cc File src/kudu/server/default-path-handlers.cc: http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/server/default-path-handlers.cc@129 PS6, Line 129: 'web' or 'all' (generalize) http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/tablet/rowset_info.h File src/kudu/tablet/rowset_info.h: http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/tablet/rowset_info.h@35 PS6, Line 35: enum class RedactMode { Can't you reuse RedactMode from flags.h? http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flag_tags.h File src/kudu/util/flag_tags.h: http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flag_tags.h@85 PS6, Line 85: in web UI in the web UI http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flag_tags.h@85 PS6, Line 85: the log message glog messages http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.h File src/kudu/util/flags.h: http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.h@60 PS6, Line 60: enum class RedactMode { I think RedactContext is a little more clear. http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.h@65 PS6, Line 65: is set with 'web' Or 'all', so just generalize this. http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc File src/kudu/util/flags.cc: http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@133 PS6, Line 133: from from the http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@148 PS6, Line 148: '' What is this referring to? http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@528 PS6, Line 528: ret_value += CheckFlagAndRedact(f, escape_mode, RedactMode::WEB); See the comment I left in GetNonDefaultFlags about passing through the redaction context). http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@553 PS6, Line 553: 'log' Or 'all', so I would just say 'if --redact indicates that log messages should be redacted'. http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@554 PS6, Line 554: string flag_value = CheckFlagAndRedact(flag, EscapeMode::NONE, RedactMode::LOG); It feels a little strange to use RedactMode::LOG when there's no requirement that callers use this function's output in a log message. I understand that currently all callers do pass the output into LOG(INFO), but maybe it'd be cleaner if RedactMode were also an argument to the function, and if RedactMode had a third option for NONE? Then the caller, knowing how the output will be used, can set the appropriate redaction context. http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h File src/kudu/util/logging.h: http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@61 PS6, Line 61: for log redaction from glogs http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@66 PS6, Line 66: for web UI redaction from the web UI http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@70 PS6, Line 70: log redaction for glogs http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@74 PS6, Line 74: for web UI redaction for the web UI http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@78 PS6, Line 78: For web UI redaction How about "Like KUDU_REDACT_WEBUI and with..." http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/test_util.cc File src/kudu/util/test_util.cc: http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/test_util.cc@87 PS6, Line 87: {"redact", "web"}, Shouldn't this be log? -- To view, visit http://gerrit.cloudera.org:8080/6755 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f Gerrit-Change-Number: 6755 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 17 Oct 2017 20:21:19 +0000 Gerrit-HasComments: Yes
