Adar Dembo has posted comments on this change. Change subject: Combine redaction flag for log and flags into one. ......................................................................
Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/6112/3/src/kudu/util/flags.cc File src/kudu/util/flags.cc: Line 112: DEFINE_string(redact, "log,flag", Please use a gflag validator (google::RegisterFlagValidator) to validate the value of the flag at runtime. Line 113: "Sensitive information that needs to be redacted (comma-separated " How about: "Comma-separated list of redactions. Supported redactions are 'flag' and 'log'. If 'flag' is specified, the values of flags tagged as sensitive will be redacted whenever server configuration is emitted. If 'log' is specified, row data will be redacted from log and error messages. Line 488: bool ShouldRedact(const string& redactionType) { Can't we just strstr(FLAGS_redact, "flag")? If/when we have redaction types that would yield false positives, we can adjust. http://gerrit.cloudera.org:8080/#/c/6112/3/src/kudu/util/flags.h File src/kudu/util/flags.h: Line 68: bool ShouldRedact(const std::string&); Nit: missing parameter name. PS3, Line 70: // 'log' redaction type. : extern const char* const redactLog; : : // 'flag' redaction type. : extern const char* const redactFlag; For callers outside flags.{cc,h} I think it'd be nicer to provide an enum for redaction types, and to pass in the enum value into ShouldRedact(). Internally ShouldRedact() can map from the enum into a string. -- To view, visit http://gerrit.cloudera.org:8080/6112 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia49ae439824351df6c87291b0828173c69ab31b0 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
