Adar Dembo has posted comments on this change. ( )

Change subject:  Add GetFlags endpoint and tool

Patch Set 1:

File src/kudu/server/
PS1, Line 74: void GenericServiceImpl::GetFlags(const GetFlagsRequestPB* req,
Could you also test this independently of the CLI?
PS1, Line 99:     flag->set_is_user_set(!entry.second.is_default);
            :     flag->set_is_default(entry.second.current_value == 
Having a hard time seeing the distinction between these two, because I had 
expected to see:


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?
File src/kudu/server/server_base.proto:
PS1, Line 47: flags match.
This doesn't make sense; did your thought get cut off?
File src/kudu/tools/
PS1, Line 2415: TEST_F(ToolTest, TestGetFlags) {
Maybe also test the tserver's endpoint?
File src/kudu/tools/tool_action_common.h:
PS1, Line 110: Status GetServerFlags(const std::string& address, uint16_t 
This isn't quite symmetric with SetServerFlag since it doesn't return the flag 
values. Perhaps rename it to PrintServerFlags?

To view, visit
To unsubscribe, visit

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 +0000
Gerrit-HasComments: Yes

Reply via email to