You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2022/05/09 11:49:04 UTC
[kudu-CR] Batch set master/tserver flags
1450306854@qq.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18506
Change subject: Batch set master/tserver flags
......................................................................
Batch set master/tserver flags
Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
3 files changed, 128 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/18506/1
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <14...@qq.com>
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18506
to look at the new patch set (#4).
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
[Config] Set all masters/tservers flags in a cluster in batch mode
Currently Kudu can only set one master/tserver's flag once a time.
This patch set all masters/tservers flags in a cluster in batch mode.
Usage:
kudu master set_all_masters_flag <master_addresses> <flag> <value>
kudu tserver set_all_tservers_flag <master_addresses> <flag> <value>
Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
3 files changed, 140 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/18506/4
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 4
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 8:
(3 comments)
> Patch Set 7:
>
> (3 comments)
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/kudu-tool-test.cc@8108
PS7, Line 8108: INSTANTIATE_TEST_SUITE_
> INSTANTIATE_TEST_CASE_P is deprecated, use INSTANTIATE_TEST_SUITE_P instead
Done
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/kudu-tool-test.cc@8109
PS7, Line 8109: sMaster)
> nit: How about 'TestSetFlagForAll'?
Done
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/tool_action_master.cc@188
PS7, Line 188: }
> nit: Should we also return RuntimeError here when failed to set flag?
Done
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
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>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 16 May 2022 03:24:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yingchun Lai, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18506
to look at the new patch set (#8).
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
[Config] Set all masters/tservers flags in a cluster in batch mode
Currently Kudu can only set one master/tserver's flag once a time.
This patch set all masters/tservers flags in a cluster in batch mode.
Usage:
kudu master set_all_masters_flag <master_addresses> <flag> <value>
kudu tserver set_all_tservers_flag <master_addresses> <flag> <value>
Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
3 files changed, 177 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/18506/8
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
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>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] Batch set master/tserver flags
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: Batch set master/tserver flags
......................................................................
Patch Set 1:
(2 comments)
The clang-tidy and IWYU checks failed, you can run these checks locally and fix them. See https://github.com/apache/kudu#running-clang-tidy-checks and https://github.com/apache/kudu#running-include-what-you-use-iwyu-checks for details.
http://gerrit.cloudera.org:8080/#/c/18506/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18506/1//COMMIT_MSG@7
PS1, Line 7: Batch set master/tserver flags
nit: Could you please add some description about what tools have been added and how these tools can be used?
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/tool_action_master.cc@793
PS1, Line 793: .AddRequiredParameter({ kValueArg, "New value for the gflag" })
Maybe we also need to provide an optional parameter 'force' here just as the set_flag tool.
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <14...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 09 May 2022 15:07:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18506
to look at the new patch set (#3).
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
[Config] Set all masters/tservers flags in a cluster in batch mode
Currently Kudu can only set one master/tserver's flag once a time.
This patch set all masters/tservers flags in a cluster in batch mode.
Usage:
kudu master set_all_masters_flag <master_addresses> <flag> <value>
kudu tserver set_all_tservers_flag <master_addresses> <flag> <value>
Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
3 files changed, 139 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/18506/3
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 3
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [Tools] Set all masters/tservers flags in a cluster in batch mode
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Tools] Set all masters/tservers flags in a cluster in batch mode
......................................................................
[Tools] Set all masters/tservers flags in a cluster in batch mode
Currently Kudu can only set one master/tserver's flag once at a time.
This patch set all masters/tservers flags in a cluster in batch mode.
Usage:
kudu master set_flag_for_all <master_addresses> <flag> <value>
kudu tserver set_flag_for_all <master_addresses> <flag> <value>
Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Reviewed-on: http://gerrit.cloudera.org:8080/18506
Tested-by: Kudu Jenkins
Reviewed-by: Yifan Zhang <ch...@163.com>
Reviewed-by: Yingchun Lai <ac...@gmail.com>
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
3 files changed, 178 insertions(+), 2 deletions(-)
Approvals:
Kudu Jenkins: Verified
Yifan Zhang: Looks good to me, approved
Yingchun Lai: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 12
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-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 4:
(8 comments)
> Patch Set 4:
>
> (16 comments)
http://gerrit.cloudera.org:8080/#/c/18506/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18506/1//COMMIT_MSG@7
PS1, Line 7: [Config] Set all masters/tservers flags in a cluster in batch mode
> nit: Could you please add some description about what tools have been added
Done
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/kudu-tool-test.cc@8137
PS1, Line 8137:
> Seems 'out' is not used here, you can pass a nullptr to RunActionStdoutStri
Done
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/kudu-tool-test.cc@8138
PS1, Line 8138: aster? opts.num_
> If going to get tserver flags, loop by opts.num_masters is meaningless.
Done
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/kudu-tool-test.cc@8150
PS1, Line 8150: &out));
: }
: rapidjson::Document doc;
: doc.Parse<0>(out.c_str());
: for (int i = 0; i < doc.Size(); i++) {
: const rapidjson::Value& item = doc[i];
: assert(item["flag"].IsString());
> Seems too complex to parse the output, you can improve the output in anothe
Done
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/tool_action_master.cc@793
PS1, Line 793: .AddRequiredParameter({ kValueArg, "New value for the gflag" })
> Maybe we also need to provide an optional parameter 'force' here just as th
Done
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/tool_action_tserver.cc@150
PS1, Line 150: if (!s.ok()) {
> How about use LOG(WARNING) ?
Done
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/tool_action_tserver.cc@154
PS1, Line 154: }
> If any tserver set failed, do not return OK.
Done
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/tool_action_tserver.cc@428
PS1, Line 428: unique_ptr<Action> set_all_tservers_flag =
> nit: remove these tail spaces.
Done
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
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-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 11 May 2022 07:08:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 9: Code-Review+1
(5 comments)
Overall looks good, just a few nits.
http://gerrit.cloudera.org:8080/#/c/18506/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18506/9//COMMIT_MSG@9
PS9, Line 9: once a time
nit: once at a time
http://gerrit.cloudera.org:8080/#/c/18506/9//COMMIT_MSG@12
PS9, Line 12: kudu master set_all_masters_flag <master_addresses> <flag> <value>
: kudu tserver set_all_tservers_flag <master_addresses> <flag> <value>
These seem to be outdated since the name for the sub-command has changed: please update.
http://gerrit.cloudera.org:8080/#/c/18506/9/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/18506/9/src/kudu/tools/kudu-tool-test.cc@8161
PS9, Line 8161: test for setting non-existed flag
how about:
A test for setting a non-existing flag.
http://gerrit.cloudera.org:8080/#/c/18506/9/src/kudu/tools/kudu-tool-test.cc@8166
PS9, Line 8166: ASSERT_TRUE(s.IsRuntimeError());
nit: any specific message to expect from the tool here (it comes in the s.ToString() string)?
http://gerrit.cloudera.org:8080/#/c/18506/9/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/18506/9/src/kudu/tools/tool_action_tserver.cc@142
PS9, Line 142: auto
nit: could this be 'const' or even 'constexpr'?
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 9
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-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 17 May 2022 03:23:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18506
to look at the new patch set (#2).
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
[Config] Set all masters/tservers flags in a cluster in batch mode
Currently Kudu can only set one master/tserver's flag once a time.
This patch set all masters/tservers flags in a cluster in batch mode.
Usage:
kudu master set_all_masters_flag <master_addresses> <flag> <value>
kudu tserver set_all_tservers_flag <master_addresses> <flag> <value>
Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
3 files changed, 139 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/18506/2
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 2
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [Tools] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Tools] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 11:
(4 comments)
> Patch Set 10: -Code-Review
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/18506/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18506/10//COMMIT_MSG@7
PS10, Line 7: Tools]
> nit: What about using 'tools' here ?
Done
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/kudu-tool-test.cc@8108
PS7, Line 8108: INSTANTIATE_TEST_SUITE_
> Done
Done
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/kudu-tool-test.cc@8109
PS7, Line 8109: estSetFl
> Well, I meant modify the second parameter. To make test_suit_name/test_name
Done
http://gerrit.cloudera.org:8080/#/c/18506/10/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
http://gerrit.cloudera.org:8080/#/c/18506/10/src/kudu/tools/tool_action_master.cc@160
PS10, Line 160: cons
> nit: Maybe this could be const as well.
Done
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 11
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-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 17 May 2022 06:51:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 9:
(2 comments)
> Patch Set 8:
>
> (2 comments)
http://gerrit.cloudera.org:8080/#/c/18506/8/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
http://gerrit.cloudera.org:8080/#/c/18506/8/src/kudu/tools/tool_action_master.cc@187
PS8, Line 187:
> nit: Align to "Set config..."
Done
http://gerrit.cloudera.org:8080/#/c/18506/8/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/18506/8/src/kudu/tools/tool_action_tserver.cc@152
PS8, Line 152: $3
> $3
Done
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 9
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-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 17 May 2022 01:26:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 10:
(5 comments)
> Patch Set 9: Code-Review+1
>
> (5 comments)
>
> Overall looks good, just a few nits.
http://gerrit.cloudera.org:8080/#/c/18506/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18506/9//COMMIT_MSG@9
PS9, Line 9: once at a t
> nit: once at a time
Done
http://gerrit.cloudera.org:8080/#/c/18506/9//COMMIT_MSG@12
PS9, Line 12: kudu master set_flag_for_all <master_addresses> <flag> <value>
: kudu tserver set_flag_for_all <master_addresses> <flag> <value>
> These seem to be outdated since the name for the sub-command has changed: p
Done
http://gerrit.cloudera.org:8080/#/c/18506/9/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/18506/9/src/kudu/tools/kudu-tool-test.cc@8161
PS9, Line 8161: A test for setting a non-existing
> how about:
Done
http://gerrit.cloudera.org:8080/#/c/18506/9/src/kudu/tools/kudu-tool-test.cc@8166
PS9, Line 8166: ASSERT_TRUE(s.IsRuntimeError());
> nit: any specific message to expect from the tool here (it comes in the s.T
Done
http://gerrit.cloudera.org:8080/#/c/18506/9/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/18506/9/src/kudu/tools/tool_action_tserver.cc@142
PS9, Line 142: cons
> nit: could this be 'const' or even 'constexpr'?
Done
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 10
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-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 17 May 2022 03:45:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 10: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/kudu-tool-test.cc@8109
PS7, Line 8109: sMaster)
> Done
Well, I meant modify the second parameter. To make test_suit_name/test_name/param_name clearer, we can rename the test like this:
class SetFlagTest {
};
INSTANTIATE_TEST_CASE_P(IsMaster, SetFlagTest, ::testing::Bool());
TEST_P(SetFlagTest, TestSetFlagForAll) {
}
http://gerrit.cloudera.org:8080/#/c/18506/10/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
http://gerrit.cloudera.org:8080/#/c/18506/10/src/kudu/tools/tool_action_master.cc@160
PS10, Line 160: auto
nit: Maybe this could be const as well.
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 10
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-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 17 May 2022 05:07:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yingchun Lai, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18506
to look at the new patch set (#9).
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
[Config] Set all masters/tservers flags in a cluster in batch mode
Currently Kudu can only set one master/tserver's flag once a time.
This patch set all masters/tservers flags in a cluster in batch mode.
Usage:
kudu master set_all_masters_flag <master_addresses> <flag> <value>
kudu tserver set_all_tservers_flag <master_addresses> <flag> <value>
Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
3 files changed, 177 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/18506/9
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 9
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-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 9: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 9
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-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 17 May 2022 02:01:25 +0000
Gerrit-HasComments: No
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yingchun Lai, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18506
to look at the new patch set (#10).
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
[Config] Set all masters/tservers flags in a cluster in batch mode
Currently Kudu can only set one master/tserver's flag once at a time.
This patch set all masters/tservers flags in a cluster in batch mode.
Usage:
kudu master set_flag_for_all <master_addresses> <flag> <value>
kudu tserver set_flag_for_all <master_addresses> <flag> <value>
Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
3 files changed, 178 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/18506/10
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 10
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-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 6:
(16 comments)
> Patch Set 4:
>
> (16 comments)
done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@18
PS4, Line 18: #include <sys/stat.
> Switch to using ASSERT_TRUE() from assert() and remove this header.
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@1197
PS4, Line 1197: };
> nit: it's better to keep this sorted alphabetically -- move this new string
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@1374
PS4, Line 1374: };
> nit: keep it sorted
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@8109
PS4, Line 8109: TEST_P(SetClusterFlagTest, IsMaster) {
> It would be great to add a test scenario to make sure that the new tools re
Ok. Add a new test for setting non-existed flag
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@8156
PS4, Line 8156: return;
> here and below: don't use assert in tests, use ASSERT_TRUE() instead
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@152
PS4, Line 152:
> I guess this list should be retrieved from the leader master instead of rel
Ok. Retrieving master addresses from leader master using RPC request.
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@154
PS4, Line 154: , &resp,
> It would help to have more information on the error per each record (e.g.,
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@155
PS4, Line 155:
> This should be std::cerr, or maybe use LOG() instead.
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@155
PS4, Line 155:
: if (resp.has_error()) {
> For better readability, use Substitute() here instead.
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@790
PS4, Line 790: most common configu
> This sub-command is already in the "master" context, so maybe rename this t
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@791
PS4, Line 791: iguration options p
> nit: for all Kudu Masters in the cluster
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@152
PS4, Line 152: LOG(WARNING)
> It would be great to see what was the error for each of these (e.g., adding
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@152
PS4, Line 152: Substitute("Set config {$0:$1} for $2 failed, error message: $1",
: flag, value, addr, s.ToString());
> For better readability, consider using Substitute() instead.
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@157
PS4, Line 157: Tablet
> nit (try to keep the same terms in the output): Tablet Server
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@429
PS4, Line 429: set_flag_for_all", &T
> That's already tserver's context (kudu tserver <sub-command>), so maybe ren
Done
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@430
PS4, Line 430: for all Kudu Tablet Server
> nit: for all Kudu Tablet Servers in the cluster
Done
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
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-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 11 May 2022 11:00:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Tools] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yingchun Lai, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18506
to look at the new patch set (#11).
Change subject: [Tools] Set all masters/tservers flags in a cluster in batch mode
......................................................................
[Tools] Set all masters/tservers flags in a cluster in batch mode
Currently Kudu can only set one master/tserver's flag once at a time.
This patch set all masters/tservers flags in a cluster in batch mode.
Usage:
kudu master set_flag_for_all <master_addresses> <flag> <value>
kudu tserver set_flag_for_all <master_addresses> <flag> <value>
Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
3 files changed, 178 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/18506/11
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 11
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-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yingchun Lai, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18506
to look at the new patch set (#5).
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
[Config] Set all masters/tservers flags in a cluster in batch mode
Currently Kudu can only set one master/tserver's flag once a time.
This patch set all masters/tservers flags in a cluster in batch mode.
Usage:
kudu master set_all_masters_flag <master_addresses> <flag> <value>
kudu tserver set_all_tservers_flag <master_addresses> <flag> <value>
Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
3 files changed, 172 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/18506/5
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
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>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yingchun Lai, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18506
to look at the new patch set (#6).
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
[Config] Set all masters/tservers flags in a cluster in batch mode
Currently Kudu can only set one master/tserver's flag once a time.
This patch set all masters/tservers flags in a cluster in batch mode.
Usage:
kudu master set_all_masters_flag <master_addresses> <flag> <value>
kudu tserver set_all_tservers_flag <master_addresses> <flag> <value>
Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
3 files changed, 172 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/18506/6
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
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-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yingchun Lai, Yifan Zhang, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18506
to look at the new patch set (#7).
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
[Config] Set all masters/tservers flags in a cluster in batch mode
Currently Kudu can only set one master/tserver's flag once a time.
This patch set all masters/tservers flags in a cluster in batch mode.
Usage:
kudu master set_all_masters_flag <master_addresses> <flag> <value>
kudu tserver set_all_tservers_flag <master_addresses> <flag> <value>
Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tserver.cc
3 files changed, 172 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/18506/7
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
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-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] Batch set master/tserver flags
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: Batch set master/tserver flags
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/kudu-tool-test.cc@8137
PS1, Line 8137: out.clear();
Seems 'out' is not used here, you can pass a nullptr to RunActionStdoutString, or use RunActionStdoutNone instead.
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/kudu-tool-test.cc@8138
PS1, Line 8138: opts.num_masters
If going to get tserver flags, loop by opts.num_masters is meaningless.
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/kudu-tool-test.cc@8150
PS1, Line 8150: vector<string> configs = Split(out, flag_key, strings::SkipEmpty());
: vector<string> values = Split(configs[1], "|", strings::SkipEmpty());
: string fvalue = values[1];
: auto itor = remove_if(fvalue.begin(),
: fvalue.end(), ::isspace);
: fvalue.erase(itor, fvalue.end());
: ASSERT_TRUE(fvalue == flag_value);
Seems too complex to parse the output, you can improve the output in another patch, for example, output in JSON format.
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/tool_action_tserver.cc@150
PS1, Line 150: std::cout << "Address: " << addr << " config: {"
How about use LOG(WARNING) ?
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/tool_action_tserver.cc@154
PS1, Line 154: return Status::OK();
If any tserver set failed, do not return OK.
http://gerrit.cloudera.org:8080/#/c/18506/1/src/kudu/tools/tool_action_tserver.cc@428
PS1, Line 428: .Build();
nit: remove these tail spaces.
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <14...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 10 May 2022 02:44:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 7:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/kudu-tool-test.cc@8108
PS7, Line 8108: INSTANTIATE_TEST_CASE_P
INSTANTIATE_TEST_CASE_P is deprecated, use INSTANTIATE_TEST_SUITE_P instead. See https://github.com/google/googletest/blob/bf66935e07825318ae519675d73d0f3e313b3ec6/googletest/include/gtest/internal/gtest-internal.h#L1303.
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/kudu-tool-test.cc@8109
PS7, Line 8109: IsMaster
nit: How about 'TestSetFlagForAll'?
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
http://gerrit.cloudera.org:8080/#/c/18506/7/src/kudu/tools/tool_action_master.cc@188
PS7, Line 188: return Status::OK();
nit: Should we also return RuntimeError here when failed to set flag?
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
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-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Fri, 13 May 2022 16:32:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Tools] Set all masters/tservers flags in a cluster in batch mode
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Tools] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 11: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 11
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-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 17 May 2022 12:20:50 +0000
Gerrit-HasComments: No
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 4:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@18
PS4, Line 18: #include <assert.h>
Switch to using ASSERT_TRUE() from assert() and remove this header.
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@1197
PS4, Line 1197: "set_all_masters_flag.*Change a gflag value",
nit: it's better to keep this sorted alphabetically -- move this new string to be after 'set_flag'
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@1374
PS4, Line 1374: "set_all_tservers_flag.*Change a gflag value",
nit: keep it sorted
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@8109
PS4, Line 8109: INSTANTIATE_TEST_CASE_P(, SetClusterFlagTest, ::testing::Bool());
It would be great to add a test scenario to make sure that the new tools return non-OK status when setting flag fail at least for one server.
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/kudu-tool-test.cc@8156
PS4, Line 8156: assert(item["flag"].IsString());
here and below: don't use assert in tests, use ASSERT_TRUE() instead
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@152
PS4, Line 152: master_addresses
I guess this list should be retrieved from the leader master instead of relying on the input. See ListMasters() in tool_action_master.cc for the reference.
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@154
PS4, Line 154: (!s.ok())
It would help to have more information on the error per each record (e.g., add s.ToString()).
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@155
PS4, Line 155: std::cout
This should be std::cerr, or maybe use LOG() instead.
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@155
PS4, Line 155: "Address: " << addr << " config: {"
: << flag << ", " << value << "} set failed!" << std::endl;
For better readability, use Substitute() here instead.
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@790
PS4, Line 790: set_all_masters_flag
This sub-command is already in the "master" context, so maybe rename this to 'set_flag_for_all' or something similar.
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_master.cc@791
PS4, Line 791: on all Kudu Masters
nit: for all Kudu Masters in the cluster
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@152
PS4, Line 152: LOG(WARNING)
It would be great to see what was the error for each of these (e.g., adding s.ToString() to each message could shed some light on that).
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@152
PS4, Line 152: "Address: " << addr << " config: {"
: << flag << ", " << value << "} set failed!"
For better readability, consider using Substitute() instead.
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@157
PS4, Line 157: TServer
nit (try to keep the same terms in the output): Tablet Server
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@429
PS4, Line 429: set_all_tservers_flag
That's already tserver's context (kudu tserver <sub-command>), so maybe rename this sub-command into 'set_flag_for_all'
http://gerrit.cloudera.org:8080/#/c/18506/4/src/kudu/tools/tool_action_tserver.cc@430
PS4, Line 430: on all Kudu Tablet Servers
nit: for all Kudu Tablet Servers in the cluster
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 4
Gerrit-Owner: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 11 May 2022 01:50:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18506/8/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
http://gerrit.cloudera.org:8080/#/c/18506/8/src/kudu/tools/tool_action_master.cc@187
PS8, Line 187: flag
nit: Align to "Set config..."
http://gerrit.cloudera.org:8080/#/c/18506/8/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/18506/8/src/kudu/tools/tool_action_tserver.cc@152
PS8, Line 152: $1
$3
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
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>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 16 May 2022 12:23:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Config] Set all masters/tservers flags in a cluster in batch mode
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Config] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 10: -Code-Review
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18506/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18506/10//COMMIT_MSG@7
PS10, Line 7: Config
nit: What about using 'tools' here ?
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 10
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-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 17 May 2022 05:13:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] [Tools] Set all masters/tservers flags in a cluster in batch mode
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18506 )
Change subject: [Tools] Set all masters/tservers flags in a cluster in batch mode
......................................................................
Patch Set 11: Code-Review+2
Thanks for adding these useful tools!
--
To view, visit http://gerrit.cloudera.org:8080/18506
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ce35389590c362d6167c677a7a3a28e08c040f
Gerrit-Change-Number: 18506
Gerrit-PatchSet: 11
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-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 17 May 2022 08:01:46 +0000
Gerrit-HasComments: No