Adar Dembo has posted comments on this change. Change subject: Combine redaction flag for log and flags into one. ......................................................................
Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/6112/7/src/kudu/util/flags.cc File src/kudu/util/flags.cc: PS7, Line 130: auto iter = RedactionUnit::redactionMap.find(value); : if (iter != RedactionUnit::redactionMap.end()) { : return true; : } You can use some of the handy functions from gutil/map-util.h for this, like ContainsKey(). But, given my other feedback, I think you should just do an if/elseif/else statement to figure out whether the values are safe. Bear in mind that you should still parse the comma-separated list for entries. You may also want to disallow "all,foo" (because if all is there, it should be there on its own). PS7, Line 140: static bool dummy [] = { : google::RegisterFlagValidator(&FLAGS_umask, &ValidateUmask), : google::RegisterFlagValidator(&FLAGS_redact, &ValidateRedact) : }; If you use the DEFINE_validator() macro, you won't need to worry about saving the result. PS7, Line 509: const char* redactFlags = FLAGS_redact.c_str(); : bool retVal; Style: we use underscores when naming local variables, not camel case. Only constants and static stuff gets the kVarName treatment. http://gerrit.cloudera.org:8080/#/c/6112/7/src/kudu/util/flags.h File src/kudu/util/flags.h: PS7, Line 73: struct RedactionUnit { : enum Type { : kRedactLog, : kRedactFlag, : kRedactAll : }; : static const char* Name(Type redactionType); : static std::unordered_map<std::string, RedactionUnit::Type> redactionMap; : }; This was more complicated than I anticipated, so let's scale it back. Maybe two functions would work? Something like this: bool ShouldRedactLog(); bool ShouldRedactFlag(); Then just call each one from wherever it needs to be called. Then there's no conversion between enums and c-strings and it should be a fair bit simpler. http://gerrit.cloudera.org:8080/#/c/6112/3/src/kudu/util/logging.h File src/kudu/util/logging.h: Line 62: // Evaluates to 'true' if the caller should redact any user data in the current scope. > Can you specify a bit more on how to do that? How would the boolean flag be The value cannot be changed; the redaction gflag isn't tagged as 'runtime'. So you'd just need to parse the main redaction flag at runtime (perhaps in the gflag validator, though maybe it's poor form for validators to have side effects) and store the result in a separate, global boolean. On second read, I do see that the original 'log_redact_user_data' flag was tagged as 'runtime'; I don't think it's a big deal to lose that feature. http://gerrit.cloudera.org:8080/#/c/6112/7/src/kudu/util/logging.h File src/kudu/util/logging.h: Line 32: using kudu::ShouldRedact; > warning: using decl 'ShouldRedact' is unused [misc-unused-using-decls] These are legit; you shouldn't put "using" statements in headers. Line 58: kudu::ScopedDisableRedaction s; \ Nit: restore this indentation? -- 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: 7 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: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
