[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Alexey Serbin has submitted this change and it was merged. Change subject: KUDU-1993: fixed validation of 'grouped' gflags .. KUDU-1993: fixed validation of 'grouped' gflags Added generic implementation for grouped gflags validators. Use CUSTOM_FLAG_VALIDATOR() macro to register late-phase validator function for command-line flags. The validation is performed upon call of HandleCommonFlags() invoked by the top-level ParseCommandLineFlags() function. Added unit test to cover the new functionality. Updated validators for security-related RPC and embedded webserver flags. As a workaround for LSAN reports on the leaks in case of ASAN build, added two scoped leak check disablers into CreateAndStartTimeoutThread. That does not seem harmful or hiding any potential leaks since it affects only the timeout thread itself. Opened separate JIRA to track the (false positive?) leak warning: KUDU-1995. Change-Id: I3755d62590cdc63a9d501ba69d980cb15f8069a9 Reviewed-on: http://gerrit.cloudera.org:8080/6795 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/rpc/messenger.cc M src/kudu/server/webserver_options.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/flag_validators-test.cc A src/kudu/util/flag_validators.cc A src/kudu/util/flag_validators.h M src/kudu/util/flags.cc M src/kudu/util/test_main.cc 8 files changed, 415 insertions(+), 36 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6795 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I3755d62590cdc63a9d501ba69d980cb15f8069a9 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Adar Dembo has posted comments on this change. Change subject: KUDU-1993: fixed validation of 'grouped' gflags .. Patch Set 7: Code-Review+2 -- 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: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Alexey Serbin has posted comments on this change. Change subject: KUDU-1993: fixed validation of 'grouped' gflags .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/6795/5/build-support/lsan-suppressions.txt File build-support/lsan-suppressions.txt: Line 30 > So either one works? Why use a suppression then? At least for TSAN, suppres It appeared to me that adding a suppression would be a cleaner way, not touching the source code. But since it seems to be temporary workaround, I think it does not matter that much. All right, let me put it back into the code (given that for TSAN supressions are more expensive that scoped leak disablers). -- 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: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6795 to look at the new patch set (#7). Change subject: KUDU-1993: fixed validation of 'grouped' gflags .. KUDU-1993: fixed validation of 'grouped' gflags Added generic implementation for grouped gflags validators. Use CUSTOM_FLAG_VALIDATOR() macro to register late-phase validator function for command-line flags. The validation is performed upon call of HandleCommonFlags() invoked by the top-level ParseCommandLineFlags() function. Added unit test to cover the new functionality. Updated validators for security-related RPC and embedded webserver flags. As a workaround for LSAN reports on the leaks in case of ASAN build, added two scoped leak check disablers into CreateAndStartTimeoutThread. That does not seem harmful or hiding any potential leaks since it affects only the timeout thread itself. Opened separate JIRA to track the (false positive?) leak warning: KUDU-1995. Change-Id: I3755d62590cdc63a9d501ba69d980cb15f8069a9 --- M src/kudu/rpc/messenger.cc M src/kudu/server/webserver_options.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/flag_validators-test.cc A src/kudu/util/flag_validators.cc A src/kudu/util/flag_validators.h M src/kudu/util/flags.cc M src/kudu/util/test_main.cc 8 files changed, 415 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/6795/7 -- To view, visit http://gerrit.cloudera.org:8080/6795 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3755d62590cdc63a9d501ba69d980cb15f8069a9 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Adar Dembo has posted comments on this change. Change subject: KUDU-1993: fixed validation of 'grouped' gflags .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/6795/5/build-support/lsan-suppressions.txt File build-support/lsan-suppressions.txt: PS5, Line 26: Adding this as a workaround since : # right now it's not clear what going on exactly > https://issues.apache.org/jira/browse/KUDU-1995 Could you add the link to lsan-suppressions.txt though? Line 30: leak:CreateAndStartTimeoutThread > It works. Updated. So either one works? Why use a suppression then? At least for TSAN, suppressions are more expensive to run than scoped disablers. -- 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: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6795 to look at the new patch set (#6). Change subject: KUDU-1993: fixed validation of 'grouped' gflags .. KUDU-1993: fixed validation of 'grouped' gflags Added generic implementation for grouped gflags validators. Use CUSTOM_FLAG_VALIDATOR() macro to register late-phase validator function for command-line flags. The validation is performed upon call of HandleCommonFlags() invoked by the top-level ParseCommandLineFlags() function. Added unit test to cover the new functionality. Updated validators for security-related RPC and embedded webserver flags. As a workaround for LSAN reports on the leaks in case of ASAN build, added suppression for CreateAndStartTimeoutThread(). That does not seem harmful or hiding any potential leaks since it affects only the timeout thread itself. Change-Id: I3755d62590cdc63a9d501ba69d980cb15f8069a9 --- M build-support/lsan-suppressions.txt M src/kudu/rpc/messenger.cc M src/kudu/server/webserver_options.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/flag_validators-test.cc A src/kudu/util/flag_validators.cc A src/kudu/util/flag_validators.h M src/kudu/util/flags.cc M src/kudu/util/test_main.cc 9 files changed, 411 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/6795/6 -- To view, visit http://gerrit.cloudera.org:8080/6795 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3755d62590cdc63a9d501ba69d980cb15f8069a9 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Alexey Serbin has posted comments on this change. Change subject: KUDU-1993: fixed validation of 'grouped' gflags .. Patch Set 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/6795/5/build-support/lsan-suppressions.txt File build-support/lsan-suppressions.txt: PS5, Line 26: Adding this as a workaround since : # right now it's not clear what going on exactly > If this warrants further investigation, could you file a JIRA and link to i https://issues.apache.org/jira/browse/KUDU-1995 PS5, Line 28: tests > Got 'tests' twice in here. Done Line 30: leak:CreateAndStartTimeoutThread() > Did the ScopedLeakCheckDisablers not work? It works. Updated. http://gerrit.cloudera.org:8080/#/c/6795/5/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: Line 157: Status s = ParseTriState("--rpc_authentication", FLAGS_rpc_authentication, &authentication); > Can we CHECK_OK() here and below? The group validators are assumed to run a Correct. We can put CHECK_OK http://gerrit.cloudera.org:8080/#/c/6795/5/src/kudu/util/flag_validators.cc File src/kudu/util/flag_validators.cc: PS5, Line 30: custom > group Done Line 37: void Register(const string& name, FlagValidator func) { > warning: the parameter 'func' is copied for each invocation but only used a Done http://gerrit.cloudera.org:8080/#/c/6795/5/src/kudu/util/flag_validators.h File src/kudu/util/flag_validators.h: PS5, Line 42: starndard > standard Done PS5, Line 55: indivigual > individual Done PS5, Line 58: this > Nit: 'the' (to be consistent with "Use the DEFINE_validator() macro...") Done Line 75: // GROUP_FLAG_VALIDATOR(groped_flags_validator, ValidateGroupedFlags); > grouped_flags_validator Done. With std::function it's not necessary to take the address, AFAIK. PS5, Line 88: an > a Done PS5, Line 88: validator > validators (or "registers a group validator") Done -- 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: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Adar Dembo has posted comments on this change. Change subject: KUDU-1993: fixed validation of 'grouped' gflags .. Patch Set 5: (11 comments) http://gerrit.cloudera.org:8080/#/c/6795/5/build-support/lsan-suppressions.txt File build-support/lsan-suppressions.txt: PS5, Line 26: Adding this as a workaround since : # right now it's not clear what going on exactly If this warrants further investigation, could you file a JIRA and link to it here? PS5, Line 28: tests Got 'tests' twice in here. Line 30: leak:CreateAndStartTimeoutThread() Did the ScopedLeakCheckDisablers not work? http://gerrit.cloudera.org:8080/#/c/6795/5/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: Line 157: Status s = ParseTriState("--rpc_authentication", FLAGS_rpc_authentication, &authentication); Can we CHECK_OK() here and below? The group validators are assumed to run after the regular gflag validators, and so if we are running this validator it should be safe to assume that --rpc_authentication and --rpc_encryption are valid, right? http://gerrit.cloudera.org:8080/#/c/6795/5/src/kudu/util/flag_validators.cc File src/kudu/util/flag_validators.cc: PS5, Line 30: custom group http://gerrit.cloudera.org:8080/#/c/6795/5/src/kudu/util/flag_validators.h File src/kudu/util/flag_validators.h: PS5, Line 42: starndard standard PS5, Line 55: indivigual individual PS5, Line 58: this Nit: 'the' (to be consistent with "Use the DEFINE_validator() macro...") Line 75: // GROUP_FLAG_VALIDATOR(groped_flags_validator, ValidateGroupedFlags); grouped_flags_validator Also, do you need to take the address of ValidateGroupedFlags (i.e. "&ValidateGroupedFlags")? PS5, Line 88: an a PS5, Line 88: validator validators (or "registers a group validator") -- 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: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6795 to look at the new patch set (#5). Change subject: KUDU-1993: fixed validation of 'grouped' gflags .. KUDU-1993: fixed validation of 'grouped' gflags Added generic implementation for grouped gflags validators. Use CUSTOM_FLAG_VALIDATOR() macro to register late-phase validator function for command-line flags. The validation is performed upon call of HandleCommonFlags() invoked by the top-level ParseCommandLineFlags() function. Added unit test to cover the new functionality. Updated validators for security-related RPC and embedded webserver flags. As a workaround for LSAN reports on the leaks in case of ASAN build, added suppression for CreateAndStartTimeoutThread(). That does not seem harmful or hiding any potential leaks since it affects only the timeout thread itself. Change-Id: I3755d62590cdc63a9d501ba69d980cb15f8069a9 --- M build-support/lsan-suppressions.txt M src/kudu/rpc/messenger.cc M src/kudu/server/webserver_options.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/flag_validators-test.cc A src/kudu/util/flag_validators.cc A src/kudu/util/flag_validators.h M src/kudu/util/flags.cc M src/kudu/util/test_main.cc 9 files changed, 414 insertions(+), 29 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/6795/5 -- To view, visit http://gerrit.cloudera.org:8080/6795 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3755d62590cdc63a9d501ba69d980cb15f8069a9 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6795 to look at the new patch set (#4). Change subject: KUDU-1993: fixed validation of 'grouped' gflags .. KUDU-1993: fixed validation of 'grouped' gflags Added generic implementation for grouped gflags validators. Use CUSTOM_FLAG_VALIDATOR() macro to register late-phase validator function for command-line flags. The validation is performed upon call of HandleCommonFlags() invoked by the top-level ParseCommandLineFlags() function. Added unit test to cover the new functionality. Updated validators for security-related RPC and embedded webserver flags. As a workaround for LSAN reports on the leaks in case of ASAN build, added ScopedLeakCheckDisabler into CreateAndStartTimeoutThread(). These do not seem to be harmful or hiding any potential leaks since they are scoped and affect only the timeout thread itself. Change-Id: I3755d62590cdc63a9d501ba69d980cb15f8069a9 --- M src/kudu/rpc/messenger.cc M src/kudu/server/webserver_options.cc M src/kudu/util/CMakeLists.txt A src/kudu/util/flag_validators-test.cc A src/kudu/util/flag_validators.cc A src/kudu/util/flag_validators.h M src/kudu/util/flags.cc M src/kudu/util/test_main.cc 8 files changed, 413 insertions(+), 29 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/6795/4 -- To view, visit http://gerrit.cloudera.org:8080/6795 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3755d62590cdc63a9d501ba69d980cb15f8069a9 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon