Adar Dembo has posted comments on this change. Change subject: [util] introduced custom gflags validators ......................................................................
Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/6795/3//COMMIT_MSG Commit Message: Line 19: This patch addresses the following JIRA item: Any particular reason your first line isn't just "KUDU-1993: introduced custom gflag validators" instead of including this additional paragraph? http://gerrit.cloudera.org:8080/#/c/6795/3/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: Line 142: DEFINE_validator(rpc_encryption, &ValidateRpcEncryption); Could we also add a DEFINE_validator() for rpc_authentication? I know it's semi-redundant with the CUSTOM_FLAG_VALIDATOR below, but I think it would be more readable if the validation were logically separate. That is: 1. Two regular validators, each enforcing that either rpc_authentication or rpc_encryption contain a legal value 2. A group validator that assumes #1 is good and checks the combination of the values. http://gerrit.cloudera.org:8080/#/c/6795/3/src/kudu/util/flag_validators-test.cc File src/kudu/util/flag_validators-test.cc: Line 71: class FlagsValidatorsDeathTest : public KuduTest { Can you explain why ASSERT_DEATH didn't work for this use case? http://gerrit.cloudera.org:8080/#/c/6795/3/src/kudu/util/flag_validators.cc File src/kudu/util/flag_validators.cc: PS3, Line 31: check What does "check" mean in this context? Line 40: CHECK(validators_.emplace(name, func).second); Can we use InsertOrDie() here instead? http://gerrit.cloudera.org:8080/#/c/6795/3/src/kudu/util/flag_validators.h File src/kudu/util/flag_validators.h: Line 30: Perhaps we should refer to these as "group" validators instead of "custom" validators? "Custom" is somewhat nebulous (especially since you can construe a regular validator as being "custom" given that it runs a custom function); "group" emphasizes that it's useful for validating groups of gflags. Line 31: #define CUSTOM_FLAG_VALIDATOR(name, func) \ Add a comment with a sample usage. Line 45: class Registrator { Doc class and constructor briefly. http://gerrit.cloudera.org:8080/#/c/6795/3/src/kudu/util/flags.cc File src/kudu/util/flags.cc: Line 415: if (!e.second()) { I think we should allow all custom validators to run before exiting. We needn't necessarily log which validators failed; it's expected that individual validators LOG something before returning false. http://gerrit.cloudera.org:8080/#/c/6795/3/src/kudu/util/test_main.cc File src/kudu/util/test_main.cc: Line 45: // The scope leak check disablers are to supress LSAN warnings in death tests. I'd like to better understand this; can you add something to the commit description explaining this? -- To view, visit http://gerrit.cloudera.org:8080/6795 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3755d62590cdc63a9d501ba69d980cb15f8069a9 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
