You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Wang Xixu (Code Review)" <ge...@cloudera.org> on 2022/11/12 13:43:24 UTC
[kudu-CR] [Flag] Check flags consistency when setting a flag
Wang Xixu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19237
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
[Flag] Check flags consistency when setting a flag
Currently Kudu will not check flags consistency
when setting a flag. When Kudu restart, this inconsistency
flags will cause restarting failed.
For example set unlock_experimental_flags=false and
auto_rebalancing_enabled=true.
Therefore this patch will check flags consistency when
setting a flag. First try to set this flag, then check
all flags consitency, if failed, roll back or return.
Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
---
M src/kudu/server/generic_service.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
4 files changed, 133 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/19237/1
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 1
Gerrit-Owner: Wang Xixu <14...@qq.com>
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19237
to look at the new patch set (#3).
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
[Flag] Check flags consistency when setting a flag
There are existing some dependencies between some flags.
For example, if setting auto_rebalancing_enabled to true,
unlock_experimental_flags must be set to true. Currently
Kudu will not check flags consistency when setting a flag
using GenericService::SetFlag() RPC. That will cause unsafe
flags or experimental flags or flag group validators unvailable.
Therefore this patch will check flags consistency when
setting a flag. First try to set this flag, then check
all flags consitency, if failed, roll back. This patch
supports to check unsafe flags, experimental flags,
flag group validators. Standard validator defined by
DEFINE_validator() will be checked when calling
google::SetCommandLineOption().
By the way, to support a group flag, for example,
rpc_certificate_file, rpc_private_key_file, and
rpc_ca_certificate_file, which all shoud be set at the same
time, this patch will add a parameter: --skip_check_consistency
to skip check consistency. There they can be set one by one.
Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
---
M src/kudu/server/generic_service.cc
M src/kudu/server/server_base.proto
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
9 files changed, 156 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/19237/3
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 3
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG
Commit Message:
PS1:
Tests are missing. Please add tests to cover the newly added functionality.
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@9
PS1, Line 9: Currently Kudu will not check flags consistency
: when setting a flag.
This sounds a bit confusing because it might be attributed to the case when just starting Kudu servers with flags set, where regular and group flag validators are run.
Please add a clarification that this is about GenericService::SetFlag() RPC.
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@10
PS1, Line 10: When Kudu restart, this inconsistency
: flags will cause restarting failed.
I'm not sure I understand what you meant by that. The flags set via GenericService::SetFlag() aren't persisted, so a restarted Kudu processes doesn't retain any settings.
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@17
PS1, Line 17: or
Should this be 'and'? I'm not sure this makes any sense as it's written now.
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 1
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 12 Nov 2022 18:42:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
Patch Set 6: Code-Review+1
(4 comments)
Overall looks good to me! Just a few nits to fix wording in comments and messages.
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/server/generic_service.cc@163
PS6, Line 163: "Roll back flag: $0 to: $1. Please set it manually."
nit: not sure the advice to set flag manually is actionable; how about
"failed to roll back flag '$0' to previous value '$1'"
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/server/server_base.proto
File src/kudu/server/server_base.proto:
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/server/server_base.proto@80
PS6, Line 80: Check all flags for consistency when a flag is set.
nit: how about
Check all flags for consistency upon setting a new value for a flag.
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/tools/tool_action_common.cc@104
PS6, Line 104: If true, kudu will check all "
: "flags consistency. If it is inconsistent, set a flag will failed.
nit: how about
If true, Kudu server checks all flags for consistency upon setting a flag. In this mode, the server rolls the flag back to its previous value and sends corresponding error response if an inconsistency is detected.
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/util/flags.h
File src/kudu/util/flags.h:
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/util/flags.h@60
PS6, Line 60: // Check unsafe flags, experimental flags, flag group validators.
: // If true, the system is in flags consistent.
nit: how about
Check for unsafe and experimental flags, and run all group flag validators. Returns 'true' if all checks pass and no inconsistencies are detected, 'false' otherwise.
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 6
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Sun, 20 Nov 2022 23:26:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19237
to look at the new patch set (#4).
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
[Flag] Check flags consistency when setting a flag
There are existing some dependencies between some flags.
For example, if setting auto_rebalancing_enabled to true,
unlock_experimental_flags must be set to true. Currently
Kudu will not check flags consistency when setting a flag
using GenericService::SetFlag() RPC. That will cause unsafe
flags or experimental flags or flag group validators unvailable.
Therefore this patch will check flags consistency when
setting a flag. First try to set this flag, then check
all flags consitency, if failed, roll back. This patch
supports to check unsafe flags, experimental flags,
flag group validators. Standard validator defined by
DEFINE_validator() will be checked when calling
google::SetCommandLineOption().
By the way, to support a group flag, for example,
rpc_certificate_file, rpc_private_key_file, and
rpc_ca_certificate_file, which all shoud be set at the same
time, this patch will add a parameter: --skip_check_consistency
to skip check consistency. There they can be set one by one.
Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
---
M src/kudu/server/generic_service.cc
M src/kudu/server/server_base.proto
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
9 files changed, 156 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/19237/4
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 4
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@15
PS1, Line 15: check flags consistency when
: setting a flag
One issue with this approach I can see is that once this consistency check is enabled, it will be impossible to change values for flags which must be set as a group. For example, take a look at --rpc_certificate_file, --rpc_private_key_file, --rpc_ca_certificate_file flags. They are not runtime as of now, but you can look at them just for an example.
To overcome that limitation once the validation is enabled, I think it makes sense to introduce a new RPC to set multiple flags at once. That can be taken care of in a separate patch.
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@551
PS1, Line 551: bool CheckFlagsInconsistence() {
> This current code misses running the regular (i.e. non-custom) validators f
After some consideration, I realized the standard validators defined by DEFINE_validator() are applied upon setting the flag upon calling google::SetCommandLineOption(), so those are not necessary to check here.
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 1
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 12 Nov 2022 22:43:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19237
to look at the new patch set (#6).
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
[Flag] Check flags consistency when setting a flag
There are existing some dependencies between some flags.
For example, if setting auto_rebalancing_enabled to true,
unlock_experimental_flags must be set to true. Currently
Kudu will not check flags consistency when setting a flag
using GenericService::SetFlag() RPC. That will cause unsafe
flags or experimental flags or flag group validators
unavailable.
Therefore this patch will check flags consistency when
setting a flag. First try to set this flag, then check
all flags consistency, if failed, roll back. This patch
supports to check unsafe flags, experimental flags,
flag group validators. Standard validator defined by
DEFINE_validator() will be checked when calling
google::SetCommandLineOption().
By the way, to support a group flag, for example,
rpc_certificate_file, rpc_private_key_file, and
rpc_ca_certificate_file, which all should be set at the same
time, this patch will add a parameter: --skip_check_consistency
to skip check consistency. There they can be set one by one.
Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
---
M src/kudu/server/generic_service.cc
M src/kudu/server/server_base.proto
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
8 files changed, 155 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/19237/6
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 6
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19237
to look at the new patch set (#5).
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
[Flag] Check flags consistency when setting a flag
There are existing some dependencies between some flags.
For example, if setting auto_rebalancing_enabled to true,
unlock_experimental_flags must be set to true. Currently
Kudu will not check flags consistency when setting a flag
using GenericService::SetFlag() RPC. That will cause unsafe
flags or experimental flags or flag group validators
unavailable.
Therefore this patch will check flags consistency when
setting a flag. First try to set this flag, then check
all flags consistency, if failed, roll back. This patch
supports to check unsafe flags, experimental flags,
flag group validators. Standard validator defined by
DEFINE_validator() will be checked when calling
google::SetCommandLineOption().
By the way, to support a group flag, for example,
rpc_certificate_file, rpc_private_key_file, and
rpc_ca_certificate_file, which all should be set at the same
time, this patch will add a parameter: --skip_check_consistency
to skip check consistency. There they can be set one by one.
Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
---
M src/kudu/server/generic_service.cc
M src/kudu/server/server_base.proto
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
8 files changed, 155 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/19237/5
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 5
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
Patch Set 3:
(13 comments)
> Patch Set 1:
>
> (2 comments)
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG
Commit Message:
PS1:
> Whoops, sorry -- I see there are some test scenarios in kudu-tool-test.cc a
Done
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@9
PS1, Line 9: There are existing some dependencies between some flags.
: For example, if sett
> This sounds a bit confusing because it might be attributed to the case when
Done
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@10
PS1, Line 10: ng auto_rebalancing_enabled to true,
: unlock_experimental_flags must be s
> I'm not sure I understand what you meant by that. The flags set via Generi
Done
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@15
PS1, Line 15:
: Therefore this
> One issue with this approach I can see is that once this consistency check
How about adding another parameter: --skip_check_consistency to skip to check all flags consistency? Then the user can set a group flags one by one. If user use the parameter skip_check_consistency, they know the risk.
Define a function to set multiple flags at once can solve the problem: set a group flags. But users may be use it to set multiple flags which are not in a flag group. At that case, shall we also check the consistency? If so, one flag set failed, shall we roll back all flags?
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@17
PS1, Line 17: t
> Should this be 'and'? I'm not sure this makes any sense as it's written no
Done
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/server/generic_service.cc@158
PS1, Line 158: string res = google::SetCommandLineOption(
> What if rolling back the flag failed? Should the server at least output a
Done
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/server/generic_service.cc@174
PS1, Line 174:
> nit: this 'return' isn't needed
Done
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/tools/kudu-tool-test.cc@4366
PS1, Line 4366: TEST_F(ToolTest, TestServerSetFlag) {
> Could you also add a scenario that tries to set valid and invalid values fo
Done
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.h
File src/kudu/util/flags.h:
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.h@60
PS1, Line 60: Check unsafe flags, experimental flags, flag group valid
> The description should include the essential information on the returned va
Done
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.h@61
PS1, Line 61: true, the system is in
> The semantics of this method is a bit confusing to me -- it returns 'true'
Done
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@443
PS1, Line 443: if (!e.second()) {
> nit: missing space after 'if'
Done
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@444
PS1, Line 444: LOG(ERROR) <
> The original code runs all the validators, and that was intentional for Run
Done
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@551
PS1, Line 551:
> After some consideration, I realized the standard validators defined by DEF
Done
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 3
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Comment-Date: Mon, 14 Nov 2022 12:14:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
Patch Set 7:
(4 comments)
> Patch Set 6: Code-Review+1
>
> (4 comments)
>
> Overall looks good to me! Just a few nits to fix wording in comments and messages.
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/server/generic_service.cc@163
PS6, Line 163: "Failed to roll back flag '$0' to previous value '$1
> nit: not sure the advice to set flag manually is actionable; how about
Done
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/server/server_base.proto
File src/kudu/server/server_base.proto:
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/server/server_base.proto@80
PS6, Line 80: Check all flags for consistency upon setting a new
> nit: how about
Done
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/tools/tool_action_common.cc@104
PS6, Line 104: If true, Kudu server checks all flags for consistency "
: "upon setting a flag. In this mode, the server rolls the flag back
> nit: how about
Done
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/util/flags.h
File src/kudu/util/flags.h:
http://gerrit.cloudera.org:8080/#/c/19237/6/src/kudu/util/flags.h@60
PS6, Line 60: // Check for unsafe and experimental flags, and run all group flag validators.
: // Returns 'true' if all checks pass and no in
> nit: how about
Done
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 7
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 21 Nov 2022 01:53:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@551
PS1, Line 551: bool CheckFlagsInconsistence() {
> This current code misses running the regular (i.e. non-custom) validators f
Ah, actually trying to set --rpc_max_message_size is better since that's a run-time flag. But checking whether it's possible to set a non-runtime flags is a good test scenario to have as well.
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 1
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 12 Nov 2022 19:43:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
Patch Set 6:
(10 comments)
> Patch Set 4:
>
> (11 comments)
http://gerrit.cloudera.org:8080/#/c/19237/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19237/4//COMMIT_MSG@14
PS4, Line 14:
> unavailable
Done
http://gerrit.cloudera.org:8080/#/c/19237/4//COMMIT_MSG@18
PS4, Line 18: flag. Firs
> consistency
Done
http://gerrit.cloudera.org:8080/#/c/19237/4//COMMIT_MSG@26
PS4, Line 26: ey_fi
> should
Done
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/generic_service.cc@156
PS4, Line 156:
> nit: flags
Done
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/generic_service.cc@162
PS4, Line 162:
> nit: rolled back
Done
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/generic_service.cc@171
PS4, Line 171: L
> nit: adding this extra scope isn't necessary, so maybe remove the curly bra
Done
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/server_base.proto
File src/kudu/server/server_base.proto:
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/server_base.proto@80
PS4, Line 80: hen a flag
> ... the flag is set.
Done
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/server_base.proto@80
PS4, Line 80: for consist
> for consistency
Done
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/server_base.proto@81
PS4, Line 81: run_consistency_check
> I think it makes sense to rename this to have a 'positive' semantics instea
Yes, I agree.
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/util/flags.cc@444
PS4, Line 444: return true;
> I guess this isn't needed since every validator prints detailed message on
Done
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 6
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 15 Nov 2022 10:39:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
Patch Set 3:
IWYU isn't yet happy: please take a look at the reported issues and fix them per http://jenkins.kudu.apache.org/job/kudu-gerrit/26708/BUILD_TYPE=IWYU/console
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 3
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 15 Nov 2022 03:12:28 +0000
Gerrit-HasComments: No
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG
Commit Message:
PS1:
> Tests are missing. Please add tests to cover the newly added functionality
Whoops, sorry -- I see there are some test scenarios in kudu-tool-test.cc actually. Please disregard this particular comment.
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/server/generic_service.cc@158
PS1, Line 158: string res = google::SetCommandLineOption(
What if rolling back the flag failed? Should the server at least output a warning about that into the logs?
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/server/generic_service.cc@174
PS1, Line 174: return;
nit: this 'return' isn't needed
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/tools/kudu-tool-test.cc@4366
PS1, Line 4366: TEST_F(ToolTest, TestServerSetFlag) {
Could you also add a scenario that tries to set valid and invalid values for a flag that has a validator defined via DEFINE_validator(), e.g. for --rpc_tls_min_protocol?
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.h
File src/kudu/util/flags.h:
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.h@60
PS1, Line 60: Verifies that the flags are allowed to be set and valid.
The description should include the essential information on the returned values.
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.h@61
PS1, Line 61: CheckFlagsInconsistence
The semantics of this method is a bit confusing to me -- it returns 'true' if there is a problem, and 'false' otherwise.
Please rename this method to follow the style guideline and make fix its semantics per https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
I'd suggest naming this AreFlagsConsistent() and return 'false' if an inconsistency is detected, 'true' otherwise.
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@443
PS1, Line 443: if(!e.second()) {
nit: missing space after 'if'
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@444
PS1, Line 444: return true;
The original code runs all the validators, and that was intentional for RunCustomValidators() -- to output information on all the detected inconsistencies at once.
http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@551
PS1, Line 551: bool CheckFlagsInconsistence() {
This current code misses running the regular (i.e. non-custom) validators for the gflags. In other words, the bug here is that the validators installed via DEFINE_validator() aren't run, and non-acceptable values for those flags might still be set.
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 1
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 12 Nov 2022 19:35:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19237
to look at the new patch set (#2).
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
[Flag] Check flags consistency when setting a flag
There are existing some dependencies between some flags.
For example, if setting auto_rebalancing_enabled to true,
unlock_experimental_flags must be set to true. Currently
Kudu will not check flags consistency when setting a flag
using GenericService::SetFlag() RPC. That will cause unsafe
flags or experimental flags or flag group validators unvailable.
Therefore this patch will check flags consistency when
setting a flag. First try to set this flag, then check
all flags consitency, if failed, roll back. This patch
supports to check unsafe flags, experimental flags,
flag group validators. Standard validator defined by
DEFINE_validator() will be checked when calling
google::SetCommandLineOption().
By the way, to support a group flag, for example,
rpc_certificate_file, rpc_private_key_file, and
rpc_ca_certificate_file, which all shoud be set at the same
time, this patch will add a parameter: --skip_check_consistency
to skip check consistency. There they can be set one by one.
Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
---
M src/kudu/server/generic_service.cc
M src/kudu/server/server_base.proto
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
9 files changed, 155 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/19237/2
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 2
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19237
to look at the new patch set (#7).
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
[Flag] Check flags consistency when setting a flag
There are existing some dependencies between some flags.
For example, if setting auto_rebalancing_enabled to true,
unlock_experimental_flags must be set to true. Currently
Kudu will not check flags consistency when setting a flag
using GenericService::SetFlag() RPC. That will cause unsafe
flags or experimental flags or flag group validators
unavailable.
Therefore this patch will check flags consistency when
setting a flag. First try to set this flag, then check
all flags consistency, if failed, roll back. This patch
supports to check unsafe flags, experimental flags,
flag group validators. Standard validator defined by
DEFINE_validator() will be checked when calling
google::SetCommandLineOption().
By the way, to support a group flag, for example,
rpc_certificate_file, rpc_private_key_file, and
rpc_ca_certificate_file, which all should be set at the same
time, this patch will add a parameter: --skip_check_consistency
to skip check consistency. There they can be set one by one.
Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
---
M src/kudu/server/generic_service.cc
M src/kudu/server/server_base.proto
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
8 files changed, 157 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/19237/7
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 7
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
Patch Set 4:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@15
PS1, Line 15:
: Therefore this
> How about adding another parameter: --skip_check_consistency to skip to che
Yes, I think it's a good idea to add an extra flag to enable/disable checking for flag consistency in SetFlag() RPC. If that looks like a preferable solution to you, I'm totally fine with that :) The only thing I'd ask is to name the newly introduced flag with positive, not negative semantics: i.e. call it something like --check_for_flags_consistency (or just --check_flags_consistency), and have it 'true' by default.
If you choose to use the route with a new RPC, as for the complications about a new RPC to set flags in groups: yes, I think we should try to set all the flags which come with a request and roll them back altogether. And I wouldn't bother too much about various misuse scenarios once the implementation is safe to rollback the previous snapshot of the flags which is consistent.
I think that the former solution is simpler and I like it better (i.e. I'd vote for the solution with the extra flag than introducing a new RPC).
http://gerrit.cloudera.org:8080/#/c/19237/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19237/4//COMMIT_MSG@14
PS4, Line 14: unvailable
unavailable
http://gerrit.cloudera.org:8080/#/c/19237/4//COMMIT_MSG@18
PS4, Line 18: consitency
consistency
http://gerrit.cloudera.org:8080/#/c/19237/4//COMMIT_MSG@26
PS4, Line 26: shoud
should
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/generic_service.cc@156
PS4, Line 156: configs
nit: flags
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/generic_service.cc@162
PS4, Line 162: Roll back
nit: rolled back
Also, consider using Substitute() to compose the message -- it's easier to read in the code
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/generic_service.cc@171
PS4, Line 171: {
nit: adding this extra scope isn't necessary, so maybe remove the curly braces at lines 171 and 178?
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/server_base.proto
File src/kudu/server/server_base.proto:
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/server_base.proto@80
PS4, Line 80: consistency
for consistency
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/server_base.proto@80
PS4, Line 80: set a flag.
... the flag is set.
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/server_base.proto@81
PS4, Line 81: skip_check_consistency
I think it makes sense to rename this to have a 'positive' semantics instead (e.g., 'run_consistency_check' instead of 'skip_consistency_check') and keep it false by default in this proto file.
The reason behind keeping it 'false' by default in the prot file is to maintain backwards compatibility for older clients (i.e. older binaries or kudu CLI tools) which aren't able to disable the consistency check otherwise.
However, the flag should be set 'true' by default when sending a request to a server in the implementation of the update 'kudu <master|tserver> set_flag' CLI tools.
Does it make sense?
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/util/flags.cc@444
PS4, Line 444: LOG(ERROR) << " Check validtor: " << e.first << " failed.";
I guess this isn't needed since every validator prints detailed message on what was wrong.
Also, it seems we might need different methods for validating the flags during SetFlag() and while running those checks upon starting up a process. For the validator targeted for SetFlag(), it makes sense to return at the first report on the flag inconsistency. However, for the validator that's run during starting up a process, it makes sense to print out all the issues detected, so it's easier to address everything right away before trying to start the process again. So, there the approach used in former revision of RunCustomValidators() should be used (e.g. something like 'found_inconsistency |= !e.second()' and continuing with the loop).
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 4
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 15 Nov 2022 03:11:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
[Flag] Check flags consistency when setting a flag
There are existing some dependencies between some flags.
For example, if setting auto_rebalancing_enabled to true,
unlock_experimental_flags must be set to true. Currently
Kudu will not check flags consistency when setting a flag
using GenericService::SetFlag() RPC. That will cause unsafe
flags or experimental flags or flag group validators
unavailable.
Therefore this patch will check flags consistency when
setting a flag. First try to set this flag, then check
all flags consistency, if failed, roll back. This patch
supports to check unsafe flags, experimental flags,
flag group validators. Standard validator defined by
DEFINE_validator() will be checked when calling
google::SetCommandLineOption().
By the way, to support a group flag, for example,
rpc_certificate_file, rpc_private_key_file, and
rpc_ca_certificate_file, which all should be set at the same
time, this patch will add a parameter: --skip_check_consistency
to skip check consistency. There they can be set one by one.
Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Reviewed-on: http://gerrit.cloudera.org:8080/19237
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/server/generic_service.cc
M src/kudu/server/server_base.proto
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
8 files changed, 157 insertions(+), 9 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 8
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] [Flag] Check flags consistency when setting a flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag
......................................................................
Patch Set 7: Code-Review+2
Thank you for the contribution!
--
To view, visit http://gerrit.cloudera.org:8080/19237
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc
Gerrit-Change-Number: 19237
Gerrit-PatchSet: 7
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 21 Nov 2022 03:08:11 +0000
Gerrit-HasComments: No