Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18441 )
Change subject: [tserver] KUDU-1827: tserver decommission ...................................................................... Patch Set 10: (13 comments) 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 Done 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 Ack 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 strings::Substitute; > nit: is this needed? Done 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: // enabled on the cluster, the master returns a signed authn token. > can you add a comment on this as well? Done 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() % 2) { > missing DECOMMISSIONED case? I'll change this back to % 2 as the existing tests start behaving in unpredictable ways if the DECOMMISSIONED state is added. http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/master/ts_state-test.cc@305 PS9, Line 305: DECOMMISSION > nit: use the full name Ack 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: }; > nit: leave the trailing comma Done http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/rebalance/rebalancer.h@303 PS9, Line 303: } // namespace kudu > nit: remove this line Done 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 Done 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? no, it was not intentional, it's placed back http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/tools/kudu-tool-test.cc@7066 PS9, Line 7066: > nit: remove these here and below Done 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: cstddef> > nit: use <cstddef> instead Done http://gerrit.cloudera.org:8080/#/c/18441/9/src/kudu/tools/tool_action_tserver.cc@368 PS9, Line 368: > nit: indent Done -- 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: 10 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 24 Jul 2023 11:39:51 +0000 Gerrit-HasComments: Yes
