You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/05/04 06:17:42 UTC
[kudu-CR] WIP: [util] introduced custom gflag validation
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6795
Change subject: WIP: [util] introduced custom gflag validation
......................................................................
WIP: [util] introduced custom gflag validation
Use CUSTOM_FLAG_VALIDATOR() macro to register late-phase validator
functions for gflags. The validation is performed upon call of
HandleCommonFlags() invoked by the top-level ParseCommandLineFlags()
function.
This patch also addresses the following JIRA item:
KUDU-1993: The validation of 'grouped' flags works incorrectly
if flags re-ordered
WIP since some tests are needed
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.cc
A src/kudu/util/flag_validators.h
M src/kudu/util/flags.cc
6 files changed, 179 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/6795/1
--
To view, visit http://gerrit.cloudera.org:8080/6795
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3755d62590cdc63a9d501ba69d980cb15f8069a9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] introduced custom gflags validators
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [util] introduced custom gflags validators
......................................................................
Patch Set 3:
(1 comment)
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
The two validators could actually be implemented as a single function, e.g. 'ValidateTriState', and applied to each flag independently.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [util] introduced custom gflags validators
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [util] introduced custom gflags validators
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 (#2).
Change subject: [util] introduced custom gflags validators
......................................................................
[util] introduced custom gflags validators
Use CUSTOM_FLAG_VALIDATOR() macro to register late-phase validator
function for 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.
This patch addresses the following JIRA item:
KUDU-1993: The validation of 'grouped' flags works incorrectly
if flags re-ordered
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
7 files changed, 349 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/6795/2
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] introduced custom gflags validators
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 (#3).
Change subject: [util] introduced custom gflags validators
......................................................................
[util] introduced custom gflags validators
Use CUSTOM_FLAG_VALIDATOR() macro to register late-phase validator
function for 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.
This patch addresses the following JIRA item:
KUDU-1993: The validation of 'grouped' flags works incorrectly
if flags re-ordered
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, 353 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/6795/3
--
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1993: fixed validation of 'grouped' gflags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>