Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15091 )
Change subject: KUDU-3011 p7: add tool to quiesce server ...................................................................... Patch Set 5: (9 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: Barrier b(num_in_parallel); How is the use of the Barrier important to the test? http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/integration-tests/maintenance_mode-itest.cc@646 PS4, Line 646: rw_workload->set_num_write_threads(3); Surround with CHECK_EQ(1, ...)? http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/integration-tests/maintenance_mode-itest.cc@766 PS4, Line 766: The larger batch size lets us increase the restart : // batch size. Tautological? Or am I missing something? 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: Status RollingRestartTServers(const vector<string> ts_to_restart) { > warning: the const qualified parameter 'ts_to_restart' is copied for each i This seems legit. 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: // Now quiesce the server. We must retry until the tool returns success, Should we first test that if we try_quiescing now, it'll fail? 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: DEFINE_int32(quiescing_request_period_secs, 1, "Period in seconds with which " : "the tool will check whether the tserver has finished quiescing."); : : DEFINE_int32(quiescing_timeout_secs, 30, "Amount of time in seconds the " : "tool will attempt to wait to quiesce a tablet server before giving up."); : Don't need anymore? http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/tools/tool_action_tserver.cc@257 PS4, Line 257: leaders Nit: maybe "tablet leaders" here and in the action description? http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/tools/tool_action_tserver.cc@389 PS4, Line 389: and Nit: or http://gerrit.cloudera.org:8080/#/c/15091/4/src/kudu/tools/tool_action_tserver.cc@389 PS4, Line 389: on-going scans Nit: active scanners, to be consistent with the error message. -- 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: 5 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: Wed, 22 Jan 2020 23:54:02 +0000 Gerrit-HasComments: Yes
