Hao Hao 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 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc@385 PS7, Line 385: shared_lock<RWMutex> l(lock_); : PathHandlerMap::const_iterator it = path_handlers > This seems a little strange; how about just heap allocating the ScopedDisab Moved the disable logic to L468 as you suggested. http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc@468 PS7, Line 468: ScopedDisableRedaction s; > Would it be sufficient to disable redaction around L468? Done http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flag_tags.h File src/kudu/util/flag_tags.h: http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flag_tags.h@84 PS7, Line 84: The values of these flags are considered sensitive and will be redacted : // if redaction is enabled. : // > Let's be a little bit vague about this, to allow the redaction implementati Done http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.h File src/kudu/util/flags.h: http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.h@68 PS7, Line 68: // Get all the flags different from their defaults. The output is a nicely : // formatted string with --flag=value pairs per line. Redact any flags that : // are tagged as sensitive, if redaction is enabled. > Update. Done http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.cc File src/kudu/util/flags.cc: http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.cc@545 PS7, Line 545: args << "--" << flag.name << '=' << flag_value; : } > Seems like this call-site ought to check g_should_redact_log and the one in On a second thought, I am not sure if it would be better to leave it more vague. As now there is not much differentiation in GetNonDefaultFlags() and CommandlineFlagsIntoString(). The enabling/disabling of web UI redaction logic is mainly controlled by ScopedDisableRedaction. http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h File src/kudu/util/logging.h: http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@63 PS7, Line 63: #define KUDU_SHOULD_REDACT() ((kudu::g_should_redact == kudu::RedactContext::ALL || \ > Shouldn't this check g_should_redact_log || g_should_redact_web? Updated. http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@91 PS7, Line 91: enum class RedactContext { > It might be clearer if the tri-state --redact values were mapped to a tri-s Updated to tri-state. http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@93 PS7, Line 93: > Nit: don't need the Done -- 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: 8 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: Mon, 23 Oct 2017 23:39:19 +0000 Gerrit-HasComments: Yes
