Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15091 )
Change subject: KUDU-3011 p7: add tool to quiesce server ...................................................................... Patch Set 7: (11 comments) http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/integration-tests/maintenance_mode-itest.cc File src/kudu/integration-tests/maintenance_mode-itest.cc: http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/integration-tests/maintenance_mode-itest.cc@501 PS4, Line 501: threads.emplace_back([&, i] { > How is the use of the Barrier important to the test? I suppose it isn't. http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/integration-tests/maintenance_mode-itest.cc@646 PS4, Line 646: } > Surround with CHECK_EQ(1, ...)? Done http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/integration-tests/maintenance_mode-itest.cc@766 PS4, Line 766: cations(1) : . > Tautological? Or am I missing something? Oops, meant RF. http://gerrit.cloudera.org:8080/#/c/15091/5/src/kudu/integration-tests/maintenance_mode-itest.cc File src/kudu/integration-tests/maintenance_mode-itest.cc: http://gerrit.cloudera.org:8080/#/c/15091/5/src/kudu/integration-tests/maintenance_mode-itest.cc@678 PS5, Line 678: // more work. > This seems legit. Done http://gerrit.cloudera.org:8080/#/c/15091/5/src/kudu/integration-tests/tablet_server_quiescing-itest.cc File src/kudu/integration-tests/tablet_server_quiescing-itest.cc: http://gerrit.cloudera.org:8080/#/c/15091/5/src/kudu/integration-tests/tablet_server_quiescing-itest.cc@401 PS5, Line 401: > Should we first test that if we try_quiescing now, it'll fail? Done http://gerrit.cloudera.org:8080/#/c/15091/3/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: http://gerrit.cloudera.org:8080/#/c/15091/3/src/kudu/tools/tool_action_tserver.cc@378 PS3, Line 378: .Description("Try quiescing the given Tablet Server. While a Tablet " > Nit: would prefer if the action name and variable matched. Makes it easier Done http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/tools/tool_action_tserver.cc@54 PS4, Line 54: : using std::cout; : using std::string; : using std::unique_ptr; : using std::vector; : > Don't need anymore? Done http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/tools/tool_action_tserver.cc@257 PS4, Line 257: kTServe > Nit: maybe "tablet leaders" here and in the action description? Done http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/tools/tool_action_tserver.cc@389 PS4, Line 389: > Nit: or Done http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/tools/tool_action_tserver.cc@389 PS4, Line 389: > Nit: active scanners, to be consistent with the error message. Done http://gerrit.cloudera.org:8080/#/c/15091/3/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/15091/3/src/kudu/tserver/tablet_service.cc@1030 PS3, Line 1030: resp->set_num_leaders(server_->num_raft_leaders()->value()); : resp->set_num_active_scanners(server_->scanner_manager()->CountActiveScanners()); : LOG(INFO) << Substitute("tserver has $0 leaders and $1 scanners", : resp->num_leaders(), resp->num_active_scanners()); : } : context->RespondSuccess(); : } : : void TabletServiceAdminImpl::CreateTablet(const CreateTabletRequestPB* req, : > Maybe simpler as: Done -- To view, visit http://gerrit.cloudera.org:8080/15091 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89657808cc2b0afc4e1b37ce75937ab12e098d9c Gerrit-Change-Number: 15091 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 23 Jan 2020 01:50:43 +0000 Gerrit-HasComments: Yes
