[kudu-CR] Add GetFlags endpoint and tool
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9948 ) Change subject: Add GetFlags endpoint and tool .. Add GetFlags endpoint and tool This adds an rpc endpoint that retrieves gflags from servers. It also includes a tool for retrieving flag values from servers. By default, it returns only flags with non-default values. It supports returning all flags, and also filtering flags by tags. Example output from the tool run against a local cluster's master: flag | value | default? | tags +---+--+- log_dir| /tmp/kudu/logs/master/0 | false | stable heap_profile_path | /tmp/kudu-master.56285| false | advanced,stable log_filename | kudu-master | false | stable webserver_interface| 127.0.0.1 | false | advanced master_addresses | 127.0.0.1:7053,127.0.0.1:7052,127.0.0.1:7051, | false | stable fs_data_dirs | /tmp/kudu/data/master/0/0 | false | stable rpc_bind_addresses | 127.0.0.1:7051| false | stable fs_wal_dir | /tmp/kudu/wal/master/0| false | stable evict_failed_followers | false | false | advanced webserver_port | 8051 | false | stable The rpc endpoint will also be used by ksck in a follow-up. Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Reviewed-on: http://gerrit.cloudera.org:8080/9948 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M src/kudu/server/generic_service.cc M src/kudu/server/generic_service.h 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_common.h 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 11 files changed, 273 insertions(+), 14 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Gerrit-Change-Number: 9948 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] Add GetFlags endpoint and tool
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9948 ) Change subject: Add GetFlags endpoint and tool .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Gerrit-Change-Number: 9948 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 10 Apr 2018 17:16:41 + Gerrit-HasComments: No
[kudu-CR] Add GetFlags endpoint and tool
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9948 to look at the new patch set (#4). Change subject: Add GetFlags endpoint and tool .. Add GetFlags endpoint and tool This adds an rpc endpoint that retrieves gflags from servers. It also includes a tool for retrieving flag values from servers. By default, it returns only flags with non-default values. It supports returning all flags, and also filtering flags by tags. Example output from the tool run against a local cluster's master: flag | value | default? | tags +---+--+- log_dir| /tmp/kudu/logs/master/0 | false | stable heap_profile_path | /tmp/kudu-master.56285| false | advanced,stable log_filename | kudu-master | false | stable webserver_interface| 127.0.0.1 | false | advanced master_addresses | 127.0.0.1:7053,127.0.0.1:7052,127.0.0.1:7051, | false | stable fs_data_dirs | /tmp/kudu/data/master/0/0 | false | stable rpc_bind_addresses | 127.0.0.1:7051| false | stable fs_wal_dir | /tmp/kudu/wal/master/0| false | stable evict_failed_followers | false | false | advanced webserver_port | 8051 | false | stable The rpc endpoint will also be used by ksck in a follow-up. Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 --- M src/kudu/server/generic_service.cc M src/kudu/server/generic_service.h 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_common.h 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 11 files changed, 273 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/9948/4 -- To view, visit http://gerrit.cloudera.org:8080/9948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Gerrit-Change-Number: 9948 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] Add GetFlags endpoint and tool
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9948 ) Change subject: Add GetFlags endpoint and tool .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/9948/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9948/3//COMMIT_MSG@17 PS3, Line 17: flag | value | default? | tags > Should we omit the 'default?' column whenever we run such that only non-def No, I think consistent output is more important for parsing than omitting a column that options may imply has a constant value. http://gerrit.cloudera.org:8080/#/c/9948/3/src/kudu/server/generic_service.cc File src/kudu/server/generic_service.cc: http://gerrit.cloudera.org:8080/#/c/9948/3/src/kudu/server/generic_service.cc@100 PS3, Line 100: flag->set_is_default(entry.second.current_value == entry.second.default_value); > entry.second.is_default would be simpler no? It's not the same thing. The google CommandLineFlagInfo struct's is_default is true if the flag has not been set-- meaning it is false if the flag has been explicitly set to its default value. The is_default of Flag PB message is true if the flag has its default value, regardless of whether it was explicitly set to that value or not. I'll rename it to is_default_value so different things have different names. http://gerrit.cloudera.org:8080/#/c/9948/3/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: http://gerrit.cloudera.org:8080/#/c/9948/3/src/kudu/tools/tool_action_common.h@107 PS3, Line 107: Gets > Nit: Prints Done -- To view, visit http://gerrit.cloudera.org:8080/9948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Gerrit-Change-Number: 9948 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 10 Apr 2018 07:48:38 + Gerrit-HasComments: Yes
[kudu-CR] Add GetFlags endpoint and tool
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9948 ) Change subject: Add GetFlags endpoint and tool .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/9948/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9948/3//COMMIT_MSG@17 PS3, Line 17: flag | value | default? | tags Should we omit the 'default?' column whenever we run such that only non-default values are returned? Or will that complicate parsing unnecessarily? http://gerrit.cloudera.org:8080/#/c/9948/3/src/kudu/server/generic_service.cc File src/kudu/server/generic_service.cc: http://gerrit.cloudera.org:8080/#/c/9948/3/src/kudu/server/generic_service.cc@100 PS3, Line 100: flag->set_is_default(entry.second.current_value == entry.second.default_value); entry.second.is_default would be simpler no? http://gerrit.cloudera.org:8080/#/c/9948/3/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: http://gerrit.cloudera.org:8080/#/c/9948/3/src/kudu/tools/tool_action_common.h@107 PS3, Line 107: Gets Nit: Prints -- To view, visit http://gerrit.cloudera.org:8080/9948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Gerrit-Change-Number: 9948 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 09 Apr 2018 17:35:47 + Gerrit-HasComments: Yes
[kudu-CR] Add GetFlags endpoint and tool
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9948 ) Change subject: Add GetFlags endpoint and tool .. Patch Set 3: Verified+1 Unrelated failure. -- To view, visit http://gerrit.cloudera.org:8080/9948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Gerrit-Change-Number: 9948 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Sat, 07 Apr 2018 20:07:45 + Gerrit-HasComments: No
[kudu-CR] Add GetFlags endpoint and tool
Will Berkeley has removed a vote on this change. Change subject: Add GetFlags endpoint and tool .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/9948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Gerrit-Change-Number: 9948 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] Add GetFlags endpoint and tool
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9948 to look at the new patch set (#3). Change subject: Add GetFlags endpoint and tool .. Add GetFlags endpoint and tool This adds an rpc endpoint that retrieves gflags from servers. It also includes a tool for retrieving flag values from servers. By default, it returns only flags with non-default values. It supports returning all flags, and also filtering flags by tags. Example output from the tool run against a local cluster's master: flag | value | default? | tags +---+--+- log_dir| /tmp/kudu/logs/master/0 | false | stable heap_profile_path | /tmp/kudu-master.56285| false | advanced,stable log_filename | kudu-master | false | stable webserver_interface| 127.0.0.1 | false | advanced master_addresses | 127.0.0.1:7053,127.0.0.1:7052,127.0.0.1:7051, | false | stable fs_data_dirs | /tmp/kudu/data/master/0/0 | false | stable rpc_bind_addresses | 127.0.0.1:7051| false | stable fs_wal_dir | /tmp/kudu/wal/master/0| false | stable evict_failed_followers | false | false | advanced webserver_port | 8051 | false | stable The rpc endpoint will also be used by ksck in a follow-up. Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 --- M src/kudu/server/generic_service.cc M src/kudu/server/generic_service.h 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_common.h 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 11 files changed, 276 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/9948/3 -- To view, visit http://gerrit.cloudera.org:8080/9948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Gerrit-Change-Number: 9948 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] Add GetFlags endpoint and tool
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9948 ) Change subject: Add GetFlags endpoint and tool .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/9948/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9948/2//COMMIT_MSG@12 PS2, Line 12: It : supports returning all flags, and also filtering flags by tags. > How difficult would it be to also filter by name via regex or something? Do Not so hard, but there's no use case for it now. Mike and I talked about it and had two use cases: - Quickly checking for non-default values from the cmd line. - ksck checking for non-default values of safe, hidden, experimental flags The default behavior + tags filtering fulfills those two cases. If somebody wants to filter by more advanced criteria they can filter the full response with all flags client-side, and if it becomes a significant use case that filtering could be added server-side later. http://gerrit.cloudera.org:8080/#/c/9948/2/src/kudu/server/generic_service.cc File src/kudu/server/generic_service.cc: http://gerrit.cloudera.org:8080/#/c/9948/2/src/kudu/server/generic_service.cc@20 PS2, Line 20: #include > warning: #includes are not sorted properly [llvm-include-order] Done -- To view, visit http://gerrit.cloudera.org:8080/9948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Gerrit-Change-Number: 9948 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Sat, 07 Apr 2018 18:37:51 + Gerrit-HasComments: Yes
[kudu-CR] Add GetFlags endpoint and tool
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9948 ) Change subject: Add GetFlags endpoint and tool .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/9948/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9948/2//COMMIT_MSG@12 PS2, Line 12: It : supports returning all flags, and also filtering flags by tags. How difficult would it be to also filter by name via regex or something? Do you think that would that be useful? Stability, default values, and (less frequently) names change from version to version so I'd imagine the more filtering power the better. That said it's probably not a pre-req to merging this patch. -- To view, visit http://gerrit.cloudera.org:8080/9948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Gerrit-Change-Number: 9948 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Sat, 07 Apr 2018 17:53:30 + Gerrit-HasComments: Yes
[kudu-CR] Add GetFlags endpoint and tool
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/9948 )
Change subject: Add GetFlags endpoint and tool
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/server/generic_service.cc@74
PS1, Line 74: void GenericServiceImpl::GetFlags(const GetFlagsRequestPB* req,
> Could you also test this independently of the CLI?
Done
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/server/generic_service.cc@99
PS1, Line 99: flag->set_is_user_set(!entry.second.is_default);
: flag->set_is_default(entry.second.current_value ==
entry.second.default_value);
> Having a hard time seeing the distinction between these two, because I had
Mmm yeah probably best.
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/server/server_base.proto
File src/kudu/server/server_base.proto:
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/server/server_base.proto@47
PS1, Line 47: flags match.
> This doesn't make sense; did your thought get cut off?
A word got eaten: "all flags match".
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/tools/kudu-tool-test.cc@2415
PS1, Line 2415: TEST_F(ToolTest, TestGetFlags) {
> Maybe also test the tserver's endpoint?
Done
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/tools/tool_action_common.h@110
PS1, Line 110: Status GetServerFlags(const std::string& address, uint16_t
default_port);
> This isn't quite symmetric with SetServerFlag since it doesn't return the f
Done
--
To view, visit http://gerrit.cloudera.org:8080/9948
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699
Gerrit-Change-Number: 9948
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Sat, 07 Apr 2018 06:58:47 +
Gerrit-HasComments: Yes
[kudu-CR] Add GetFlags endpoint and tool
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9948 to look at the new patch set (#2). Change subject: Add GetFlags endpoint and tool .. Add GetFlags endpoint and tool This adds an rpc endpoint that retrieves gflags from servers. It also includes a tool for retrieving flag values from servers. By default, it returns only flags with non-default values. It supports returning all flags, and also filtering flags by tags. Example output from the tool run against a local cluster's master: flag | value | default? | tags +---+--+- log_dir| /tmp/kudu/logs/master/0 | false | stable heap_profile_path | /tmp/kudu-master.56285| false | advanced,stable log_filename | kudu-master | false | stable webserver_interface| 127.0.0.1 | false | advanced master_addresses | 127.0.0.1:7053,127.0.0.1:7052,127.0.0.1:7051, | false | stable fs_data_dirs | /tmp/kudu/data/master/0/0 | false | stable rpc_bind_addresses | 127.0.0.1:7051| false | stable fs_wal_dir | /tmp/kudu/wal/master/0| false | stable evict_failed_followers | false | false | advanced webserver_port | 8051 | false | stable The rpc endpoint will also be used by ksck in a follow-up. Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 --- M src/kudu/server/generic_service.cc M src/kudu/server/generic_service.h 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_common.h 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 11 files changed, 274 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/9948/2 -- To view, visit http://gerrit.cloudera.org:8080/9948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Gerrit-Change-Number: 9948 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] Add GetFlags endpoint and tool
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/9948 )
Change subject: Add GetFlags endpoint and tool
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/server/generic_service.cc@74
PS1, Line 74: void GenericServiceImpl::GetFlags(const GetFlagsRequestPB* req,
Could you also test this independently of the CLI?
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/server/generic_service.cc@99
PS1, Line 99: flag->set_is_user_set(!entry.second.is_default);
: flag->set_is_default(entry.second.current_value ==
entry.second.default_value);
Having a hard time seeing the distinction between these two, because I had
expected to see:
flag->set_is_default(entry.second.is_default);
Based on my reading of gflags.h, it seems like is_user_set() isn't really
something we can determine with certainty. We can only tell that a flag's value
is non-default, but we don't know whether that's because Kudu internally
changed it, or because the user did on the command line. You talk about this in
the commit message; maybe we should drop is_user_set() since we can't provide
it consistently?
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/server/server_base.proto
File src/kudu/server/server_base.proto:
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/server/server_base.proto@47
PS1, Line 47: flags match.
This doesn't make sense; did your thought get cut off?
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/tools/kudu-tool-test.cc@2415
PS1, Line 2415: TEST_F(ToolTest, TestGetFlags) {
Maybe also test the tserver's endpoint?
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
http://gerrit.cloudera.org:8080/#/c/9948/1/src/kudu/tools/tool_action_common.h@110
PS1, Line 110: Status GetServerFlags(const std::string& address, uint16_t
default_port);
This isn't quite symmetric with SetServerFlag since it doesn't return the flag
values. Perhaps rename it to PrintServerFlags?
--
To view, visit http://gerrit.cloudera.org:8080/9948
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699
Gerrit-Change-Number: 9948
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 06 Apr 2018 21:24:07 +
Gerrit-HasComments: Yes
[kudu-CR] Add GetFlags endpoint and tool
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9948 Change subject: Add GetFlags endpoint and tool .. Add GetFlags endpoint and tool This adds an rpc endpoint that retrieves gflags from servers. It also includes a tool for retrieving flag values from servers. By default, it returns only flags that were explicitly set (according to gflags). It supports returning all flags, and also filtering flags by tags. Example output from the tool run against a local cluster's master: flag | value | set? | default value? | tags +---+--++- log_dir| /tmp/kudu/logs/master/0 | true | false | stable heap_profile_path | /tmp/kudu-master.56285| true | false | advanced,stable log_filename | kudu-master | true | false | stable webserver_doc_root | /Users/wdberkeley/src/kudu/www| true | true | advanced webserver_interface| 127.0.0.1 | true | false | advanced master_addresses | 127.0.0.1:7053,127.0.0.1:7052,127.0.0.1:7051, | true | false | stable fs_data_dirs | /tmp/kudu/data/master/0/0 | true | false | stable rpc_bind_addresses | 127.0.0.1:7051| true | false | stable fs_wal_dir | /tmp/kudu/wal/master/0| true | false | stable evict_failed_followers | false | true | false | advanced webserver_port | 8051 | true | false | stable Unfortunately, some flags are set within Kudu and it's not possible for gflags to determine that they weren't set with an explicit flag, e.g. -log_filename and -evict_failed_followers in the above output. Still, the output is valuable for quickly seeing a server's configuration. The rpc endpoint will also be used by ksck in a follow-up. Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 --- M src/kudu/server/generic_service.cc M src/kudu/server/generic_service.h 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_common.h 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 10 files changed, 200 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/9948/1 -- To view, visit http://gerrit.cloudera.org:8080/9948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia35b4261099c1a3c6e2ff68e907c84df9a7ff699 Gerrit-Change-Number: 9948 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley
