You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2019/10/04 02:05:08 UTC

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI 

Hello Alexey Serbin,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/14369

to review the following change.


Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................

KUDU-2069 p8: support listing tserver state through CLI

This patch adds support to the tserver list tool to show the tserver
states.

$ kudu tserver list <master> --columns=uuid,state

Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
---
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/ts_manager.cc
M src/kudu/master/ts_manager.h
M src/kudu/tools/tool_action_tserver.cc
5 files changed, 67 insertions(+), 4 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/14369/1
-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14369

to look at the new patch set (#3).

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................

KUDU-2069 p8: support listing tserver state through CLI

This patch adds support to the tserver list tool to show the tserver
states.

$ kudu tserver list <master> --columns=uuid,state

I opted to have the state be an optional return field because there are
cases where a tablet that is unregistered will have a state associated
with it (e.g. if the master is restarted), in which case it may be
surprising to see info about them, unless explicitly requested.

Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
---
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/ts_manager.cc
M src/kudu/master/ts_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_tserver.cc
6 files changed, 106 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/14369/3
-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14369 )

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc@574
PS2, Line 574:   for (const auto& ts_and_state : states) {
             :     const auto& ts = ts_and_state.first;
             :     const auto& state = ts_and_state.second;
             :     ListTabletServersResponsePB::Entry* entry = resp->add_servers();
             :     NodeInstancePB* instance = entry->mutable_instance_id();
             :     instance->set_permanent_uuid(ts);
             :     instance->set_instance_seqno(-1);
             :     entry->set_state(state);
What are possible consumers of this information?

If including such a bogus info, maybe explicitly declare such behavior in the .proto file?  And from the compatibility standpoint, what would older clients might do if they receive such info?



-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 04 Oct 2019 21:16:00 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14369 )

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc@563
PS2, Line 563:     const auto& uuid = desc->permanent_uuid();
Any reason not to always return the state?



-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 04 Oct 2019 20:52:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14369 )

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14369/3/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:

http://gerrit.cloudera.org:8080/#/c/14369/3/src/kudu/tools/tool_action_tserver.cc@64
PS3, Line 64: using master::TServerStatePB;
> warning: using decl 'TServerStatePB' is unused [misc-unused-using-decls]
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 08 Oct 2019 22:02:07 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14369 )

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc@564
PS2, Line 564:     const auto* state = FindOrNull(states, uuid);
Could you add a comment explaining when we expect to see null here? Is it just for tservers with a NONE state? Or other cases too?


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc@577
PS2, Line 577:     ListTabletServersResponsePB::Entry* entry = resp->add_servers();
What about the other entry fields? Any worth setting to something bogus? Thinking millis_since_heartbeat might be interesting (default value is 0, right?).


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/ts_manager.cc
File src/kudu/master/ts_manager.cc:

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/ts_manager.cc@255
PS2, Line 255:     LOG(INFO) << Substitute("Unset tserver state for $0",
             :                             ts_uuid, TServerStatePB_Name(ts_state));
I asked about this in the previous patch; maybe move it there?


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/kudu-tool-test.cc@2918
PS2, Line 2918: TEST_F(ToolTest, TestTServerListState) {
Could you test the "state exists but there's otherwise no entry" case? Maybe enter_maintenance on the tserver, stop it, and restart the master? Would that do it?


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/tool_action_tserver.cc@73
PS2, Line 73: string TServerStateToString(TServerStatePB state) {
If you feel lazy, you could get away with using TServerStatePB_Name instead. Then we won't have to update this function when we add new states too.


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/tool_action_tserver.cc@120
PS2, Line 120: 
             :   proxy.SyncRpc<ListTabletServersRequestPB, ListTabletServersResponsePB>(
             :       req, &resp, "ListTabletServers", &MasterServiceProxy::ListTabletServersAsync);
Wrap and check for errors? In fact, could you audit all SyncRpc calls?


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/tool_action_tserver.cc@135
PS2, Line 135: strings::Split(FLAGS_columns, ",", strings::SkipEmpty()
Can reuse cols here.


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/tool_action_tserver.cc@176
PS2, Line 176:         values.emplace_back(server.has_state() ? TServerStateToString(server.state()) : "<none>");
Maybe instead:

  values.emplace_back(TServerStateToString(server.has_state() ? server.state() : TServerStatePB::None));

That way "<none>" is only a magic string in one place.


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/tool_action_tserver.cc@203
PS2, Line 203:   Status s = proxy.SyncRpc<ChangeTServerStateRequestPB, ChangeTServerStateResponsePB>(
             :       req, &resp, "ChangeTServerState", &MasterServiceProxy::ChangeTServerStateAsync);
             :   RETURN_NOT_OK(s);
Combine?

But doesn't this belong in the previous patch?



-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 05 Oct 2019 01:31:39 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14369

to look at the new patch set (#2).

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................

KUDU-2069 p8: support listing tserver state through CLI

This patch adds support to the tserver list tool to show the tserver
states.

$ kudu tserver list <master> --columns=uuid,state

Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
---
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/ts_manager.cc
M src/kudu/master/ts_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_tserver.cc
6 files changed, 97 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/14369/2
-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14369 )

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14369/4/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/14369/4/src/kudu/master/master_service.cc@566
PS4, Line 566:     const auto& state = FindWithDefault(states, uuid, TServerStatePB::NONE);
Can't store in a ref here. From FindWithDefault's doc:

  // WARNING: If a temporary object is passed as the default "value," this
  // function will return a reference to that temporary object, which will be
  // destroyed by the end of the statement. Specifically, if you have a map with
  // string values, and you pass a char* as the default "value," either use the
  // returned value immediately or store it in a string (not string&). Details:



-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 08 Oct 2019 22:09:11 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14369 )

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc@563
PS2, Line 563:     const auto& uuid = desc->permanent_uuid();
> Any reason not to always return the state?
States may exist for tablet servers that have not yet registered, so it might be surprising to see info about them except in the case where the state is requested.


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc@574
PS2, Line 574:   for (const auto& ts_and_state : states) {
             :     const auto& ts = ts_and_state.first;
             :     const auto& state = ts_and_state.second;
             :     ListTabletServersResponsePB::Entry* entry = resp->add_servers();
             :     NodeInstancePB* instance = entry->mutable_instance_id();
             :     instance->set_permanent_uuid(ts);
             :     instance->set_instance_seqno(-1);
             :     entry->set_state(state);
> What are possible consumers of this information?
Ack

Older clients wouldn't see anything, because they wouldn't use include_states in the request.



-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 04 Oct 2019 21:19:36 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14369 )

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................

KUDU-2069 p8: support listing tserver state through CLI

This patch adds support to the tserver list tool to show the tserver
states.

$ kudu tserver list <master> --columns=uuid,state

I opted to have the state be an optional return field because there are
cases where a tablet that is unregistered will have a state associated
with it (e.g. if the master is restarted), in which case it may be
surprising to see info about them, unless explicitly requested.

Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Reviewed-on: http://gerrit.cloudera.org:8080/14369
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/ts_manager.cc
M src/kudu/master/ts_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_tserver.cc
6 files changed, 108 insertions(+), 8 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14369 )

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14369/4/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/14369/4/src/kudu/master/master_service.cc@566
PS4, Line 566:     const auto state = FindWithDefault(states, uuid, TServerStatePB::NONE);
> Can't store in a ref here. From FindWithDefault's doc:
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 08 Oct 2019 22:36:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14369 )

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................


Patch Set 3:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc@564
PS2, Line 564:     const auto& state = FindWithDefault(states, uuid, TServerStatePB::NONE);
> Could you add a comment explaining when we expect to see null here? Is it j
Done


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/master_service.cc@577
PS2, Line 577:     instance->set_instance_seqno(-1);
> What about the other entry fields? Any worth setting to something bogus? Th
Done


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/ts_manager.cc
File src/kudu/master/ts_manager.cc:

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/ts_manager.cc@62
PS2, Line 62: using std::shared_ptr;
> warning: using decl 'unordered_map' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/master/ts_manager.cc@255
PS2, Line 255:     RETURN_NOT_OK_PREPEND(sys_catalog->RemoveTServerState(ts_uuid),
             :         Substitute("Failed to remove tserver state for $0", ts_uuid)
> I asked about this in the previous patch; maybe move it there?
Done


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/kudu-tool-test.cc@2918
PS2, Line 2918: TEST_F(ToolTest, TestTServerListState) {
> Could you test the "state exists but there's otherwise no entry" case? Mayb
Done


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:

http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/tool_action_tserver.cc@73
PS2, Line 73: const char* const kTServerIdArg = "tserver_uuid";
> If you feel lazy, you could get away with using TServerStatePB_Name instead
Done


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/tool_action_tserver.cc@120
PS2, Line 120: 
             :   DataTable table({});
             :   const auto& servers = resp.servers();
> Wrap and check for errors? In fact, could you audit all SyncRpc calls?
Will do in a separate patch. For some reason I had to wrap in an extra ().


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/tool_action_tserver.cc@135
PS2, Line 135: r : servers) {
> Can reuse cols here.
Done


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/tool_action_tserver.cc@176
PS2, Line 176: 
> Maybe instead:
Done


http://gerrit.cloudera.org:8080/#/c/14369/2/src/kudu/tools/tool_action_tserver.cc@203
PS2, Line 203:   return Status::OK();
             : }
             : 
> Combine?
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 08 Oct 2019 21:08:41 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14369

to look at the new patch set (#4).

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................

KUDU-2069 p8: support listing tserver state through CLI

This patch adds support to the tserver list tool to show the tserver
states.

$ kudu tserver list <master> --columns=uuid,state

I opted to have the state be an optional return field because there are
cases where a tablet that is unregistered will have a state associated
with it (e.g. if the master is restarted), in which case it may be
surprising to see info about them, unless explicitly requested.

Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
---
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/ts_manager.cc
M src/kudu/master/ts_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_tserver.cc
6 files changed, 108 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/14369/4
-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14369 )

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................


Patch Set 5: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 08 Oct 2019 22:39:03 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2069 p8: support listing tserver state through CLI

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14369

to look at the new patch set (#5).

Change subject: KUDU-2069 p8: support listing tserver state through CLI
......................................................................

KUDU-2069 p8: support listing tserver state through CLI

This patch adds support to the tserver list tool to show the tserver
states.

$ kudu tserver list <master> --columns=uuid,state

I opted to have the state be an optional return field because there are
cases where a tablet that is unregistered will have a state associated
with it (e.g. if the master is restarted), in which case it may be
surprising to see info about them, unless explicitly requested.

Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
---
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/ts_manager.cc
M src/kudu/master/ts_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_tserver.cc
6 files changed, 108 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/14369/5
-- 
To view, visit http://gerrit.cloudera.org:8080/14369
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5917927f3bc4bb6bebd1cfb63555ec20d94d9091
Gerrit-Change-Number: 14369
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)