Alexey Serbin has posted comments on this change. Change subject: KUDU-1993: fixed validation of 'grouped' gflags ......................................................................
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 cus Nothing in particular. Probably, that's because of some self-confusion because I thought mostly about custom validators functionality, and also fixed KUDU-1993 in the same patch. 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 That's a good observation. I agree it would be cleaner that way. 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? As I understand, ASSERT_DEATH() asserts that the process crashes due to the call of the 'unexpected' handler of the C++ run-time (e.g., it's is called on non-handled exception getting to the very top-level). In this case, the process calls exit() explicitly, so ASSERT_EXIT/EXPECT_EXIT seem more appropriate. https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#how-to-write-a-death-test 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? it was meant 'consider using'. I think this comment is mis-placed: the appropriate place for this is in the comment for the GROUP_VALIDATOR_MACRO(). I'll remove this. Line 40: CHECK(validators_.emplace(name, func).second); > Can we use InsertOrDie() here instead? Done 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" ok, GROUP_FLAG_VALIDATOR() it will be :) Line 31: #define CUSTOM_FLAG_VALIDATOR(name, func) \ > Add a comment with a sample usage. Done Line 45: class Registrator { > Doc class and constructor briefly. Done 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 nee Yep, that's a better approach. 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 des This is just a work-around for LSAN leaks. Frankly, I'm not happy about this, but that was the easier way to get it passing the ASAN build if running those death tests. It would be nice to clean this up. I'll add a note into the commit message. -- 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: Alexey Serbin <aser...@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
