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 <wdberke...@gmail.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 06 Apr 2018 21:24:07 +0000
Gerrit-HasComments: Yes

Reply via email to