[kudu-CR] [tserver] KUDU-1827: tserver decommission

["Attila Bukor (Code Review)" <ge...@cloudera.org>]

Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18441 )

Change subject: [tserver] KUDU-1827: tserver decommission
......................................................................


Patch Set 9:

(13 comments)

Thanks for the contribution. I did a quick first pass, overall it looks good, but there are a few nits.

http://gerrit.cloudera.org:8080/#/c/18441/9//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/18441/9//COMMIT_MSG@11
PS9, Line 11: 	* DECOMMISSIONING_IN_PROGRESS
nit: use spaces instead of tabs


http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/integration-tests/CMakeLists.txt
File src/kudu/integration-tests/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/integration-tests/CMakeLists.txt@34
PS9, Line 34:   ts_itest-base.cc)
nit: this change is unnecessary


http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/master/auto_rebalancer-test.cc
File src/kudu/master/auto_rebalancer-test.cc:

http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/master/auto_rebalancer-test.cc@70
PS9, Line 70: using kudu::master::MasterServiceProxy;
nit: is this needed?


http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/master/master.proto@956
PS9, Line 956:   DECOMMISSIONED = 4;
can you add a comment on this as well?


http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/master/ts_state-test.cc
File src/kudu/master/ts_state-test.cc:

http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/master/ts_state-test.cc@69
PS9, Line 69:   switch (rand() % 3) {
missing DECOMMISSIONED case?


http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/master/ts_state-test.cc@305
PS9, Line 305: deco_in_prog
nit: use the full name


http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/rebalance/rebalancer.h
File src/kudu/rebalance/rebalancer.h:

http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/rebalance/rebalancer.h@163
PS9, Line 163:     TIMED_OUT
nit: leave the trailing comma


http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/rebalance/rebalancer.h@303
PS9, Line 303: 
nit: remove this line


http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/rebalance/rebalancer.cc
File src/kudu/rebalance/rebalancer.cc:

http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/rebalance/rebalancer.cc@544
PS9, Line 544:  
nit: extra space


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

http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/tools/kudu-tool-test.cc@a7788
PS9, Line 7788: 
Is this removed intentionally? If yes, why?


http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/tools/kudu-tool-test.cc@7066
PS9, Line 7066:   std::cout << std::endl << output << std::endl;
nit: remove these here and below


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

http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/tools/tool_action_tserver.cc@18
PS9, Line 18: stddef.h
nit: use <cstddef> instead


http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/tools/tool_action_tserver.cc@368
PS9, Line 368:   req, &resp, "ListTabletServers", &MasterServiceProxy::ListTabletServersAsync)));
nit: indent



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15c52b653c20b2a3a45fbf8934f19f6bd1a9caea
Gerrit-Change-Number: 18441
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 11 May 2022 23:36:33 +0000
Gerrit-HasComments: Yes