You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "ZhangYao (Code Review)" <ge...@cloudera.org> on 2019/10/17 16:47:09 UTC
[kudu-CR] [tools] Add options to get specific flags.
ZhangYao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14486
Change subject: [tools] Add options to get specific flags.
......................................................................
[tools] Add options to get specific flags.
Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
---
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
6 files changed, 58 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/14486/1
--
To view, visit http://gerrit.cloudera.org:8080/14486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
Gerrit-Change-Number: 14486
Gerrit-PatchSet: 1
Gerrit-Owner: ZhangYao <tr...@gmail.com>
[kudu-CR] [tools] Add options to get specific flags.
Posted by "ZhangYao (Code Review)" <ge...@cloudera.org>.
ZhangYao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14486 )
Change subject: [tools] Add options to get specific flags.
......................................................................
Patch Set 2:
(3 comments)
> (3 comments)
>
> Could you also update the GetFlags test in tablet_server-test?
Added :)
http://gerrit.cloudera.org:8080/#/c/14486/1/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/14486/1/src/kudu/server/generic_service.cc@81
PS1, Line 81: // If no tags were specified, return all flags that have non-default values.
> This should be documented in the proto definition too.
Done
http://gerrit.cloudera.org:8080/#/c/14486/1/src/kudu/server/generic_service.cc@84
PS1, Line 84: bool all_flags = req->all_flags();
> I think you can do:
Nice :) done.
http://gerrit.cloudera.org:8080/#/c/14486/1/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/14486/1/src/kudu/tools/tool_action_common.cc@122
PS1, Line 122: An empty value means no restriction. "
: "If non-empty, all_flags is ignored.");
> "An empty value means no restriction. If non-empty, all_flags is ignored."
Done
--
To view, visit http://gerrit.cloudera.org:8080/14486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
Gerrit-Change-Number: 14486
Gerrit-PatchSet: 2
Gerrit-Owner: ZhangYao <tr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: ZhangYao <tr...@gmail.com>
Gerrit-Comment-Date: Fri, 18 Oct 2019 14:01:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add options to get specific flags.
Posted by "ZhangYao (Code Review)" <ge...@cloudera.org>.
ZhangYao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14486 )
Change subject: [tools] Add options to get specific flags.
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14486/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/14486/2/src/kudu/tserver/tablet_server-test.cc@356
PS2, Line 356: [](const server::GetFlagsResponsePB::Flag& flag) -> bool {
: return flag.name() == "help";
: }))
> Needs one more indent.
Done
http://gerrit.cloudera.org:8080/#/c/14486/2/src/kudu/tserver/tablet_server-test.cc@360
PS2, Line 360: [](const server::GetFlagsResponsePB::Flag& flag) -> bool {
: return flag.name() == "logemaillevel";
: }));
> Needs one less indent.
Done
http://gerrit.cloudera.org:8080/#/c/14486/2/src/kudu/tserver/tablet_server-test.cc@374
PS2, Line 374: [](const server::GetFlagsResponsePB::Flag& flag) -> bool {
: return flag.name() == "log_dir";
: }))
> Needs one more indent.
Done
http://gerrit.cloudera.org:8080/#/c/14486/2/src/kudu/tserver/tablet_server-test.cc@404
PS2, Line 404: [](const server::GetFlagsResponsePB::Flag& flag) -> bool {
: return flag.name() == "logemaillevel";
: }))
> Nit: needs to be indented by one more character.
Done
--
To view, visit http://gerrit.cloudera.org:8080/14486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
Gerrit-Change-Number: 14486
Gerrit-PatchSet: 3
Gerrit-Owner: ZhangYao <tr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: ZhangYao <tr...@gmail.com>
Gerrit-Comment-Date: Sat, 19 Oct 2019 10:10:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add options to get specific flags.
Posted by "ZhangYao (Code Review)" <ge...@cloudera.org>.
ZhangYao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14486 )
Change subject: [tools] Add options to get specific flags.
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
Gerrit-Change-Number: 14486
Gerrit-PatchSet: 3
Gerrit-Owner: ZhangYao <tr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: ZhangYao <tr...@gmail.com>
Gerrit-Comment-Date: Sat, 19 Oct 2019 12:03:00 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Add options to get specific flags.
Posted by "ZhangYao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14486
to look at the new patch set (#3).
Change subject: [tools] Add options to get specific flags.
......................................................................
[tools] Add options to get specific flags.
Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
---
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
7 files changed, 121 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/14486/3
--
To view, visit http://gerrit.cloudera.org:8080/14486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
Gerrit-Change-Number: 14486
Gerrit-PatchSet: 3
Gerrit-Owner: ZhangYao <tr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: ZhangYao <tr...@gmail.com>
[kudu-CR] [tools] Add options to get specific flags.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14486 )
Change subject: [tools] Add options to get specific flags.
......................................................................
[tools] Add options to get specific flags.
Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
Reviewed-on: http://gerrit.cloudera.org:8080/14486
Tested-by: Kudu Jenkins
Reviewed-by: ZhangYao <tr...@gmail.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
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
7 files changed, 121 insertions(+), 2 deletions(-)
Approvals:
Kudu Jenkins: Verified
ZhangYao: Looks good to me, approved
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
Gerrit-Change-Number: 14486
Gerrit-PatchSet: 4
Gerrit-Owner: ZhangYao <tr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: ZhangYao <tr...@gmail.com>
[kudu-CR] [tools] Add options to get specific flags.
Posted by "ZhangYao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14486
to look at the new patch set (#2).
Change subject: [tools] Add options to get specific flags.
......................................................................
[tools] Add options to get specific flags.
Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
---
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
7 files changed, 121 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/14486/2
--
To view, visit http://gerrit.cloudera.org:8080/14486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
Gerrit-Change-Number: 14486
Gerrit-PatchSet: 2
Gerrit-Owner: ZhangYao <tr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [tools] Add options to get specific flags.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14486 )
Change subject: [tools] Add options to get specific flags.
......................................................................
Patch Set 1:
(3 comments)
Could you also update the GetFlags test in tablet_server-test?
http://gerrit.cloudera.org:8080/#/c/14486/1/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/14486/1/src/kudu/server/generic_service.cc@81
PS1, Line 81: // If flags were specified, will ignore 'all_flags'.
This should be documented in the proto definition too.
http://gerrit.cloudera.org:8080/#/c/14486/1/src/kudu/server/generic_service.cc@84
PS1, Line 84: unordered_set<string> flags;
I think you can do:
unordered_set<string> flags(req->flags().begin(), req->flags().end());
http://gerrit.cloudera.org:8080/#/c/14486/1/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/14486/1/src/kudu/tools/tool_action_common.cc@122
PS1, Line 122: Once set it will ignore the 'all_flags'. "
: "An empty value means this config is not used")
"An empty value means no restriction. If non-empty, all_flags is ignored."
--
To view, visit http://gerrit.cloudera.org:8080/14486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
Gerrit-Change-Number: 14486
Gerrit-PatchSet: 1
Gerrit-Owner: ZhangYao <tr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 17 Oct 2019 18:45:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add options to get specific flags.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14486 )
Change subject: [tools] Add options to get specific flags.
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
Gerrit-Change-Number: 14486
Gerrit-PatchSet: 3
Gerrit-Owner: ZhangYao <tr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: ZhangYao <tr...@gmail.com>
Gerrit-Comment-Date: Mon, 21 Oct 2019 04:40:44 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Add options to get specific flags.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14486 )
Change subject: [tools] Add options to get specific flags.
......................................................................
Patch Set 2: Code-Review+2
(4 comments)
Failures were due to NTP sync.
Feel free to fix my nits and then give yourself a +2.
http://gerrit.cloudera.org:8080/#/c/14486/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/14486/2/src/kudu/tserver/tablet_server-test.cc@356
PS2, Line 356: [](const server::GetFlagsResponsePB::Flag& flag) -> bool {
: return flag.name() == "help";
: }));
Needs one more indent.
http://gerrit.cloudera.org:8080/#/c/14486/2/src/kudu/tserver/tablet_server-test.cc@360
PS2, Line 360: [](const server::GetFlagsResponsePB::Flag& flag) -> bool {
: return flag.name() == "logemaillevel";
: }));
Needs one less indent.
http://gerrit.cloudera.org:8080/#/c/14486/2/src/kudu/tserver/tablet_server-test.cc@374
PS2, Line 374: [](const server::GetFlagsResponsePB::Flag& flag) -> bool {
: return flag.name() == "log_dir";
: }));
Needs one more indent.
http://gerrit.cloudera.org:8080/#/c/14486/2/src/kudu/tserver/tablet_server-test.cc@404
PS2, Line 404: [](const server::GetFlagsResponsePB::Flag& flag) -> bool {
: return flag.name() == "logemaillevel";
: }));
Nit: needs to be indented by one more character.
--
To view, visit http://gerrit.cloudera.org:8080/14486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id51ac07ffae54cd1dee25921612934e84d505f24
Gerrit-Change-Number: 14486
Gerrit-PatchSet: 2
Gerrit-Owner: ZhangYao <tr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: ZhangYao <tr...@gmail.com>
Gerrit-Comment-Date: Fri, 18 Oct 2019 17:37:57 +0000
Gerrit-HasComments: Yes