Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9510 )
Change subject: [tools] KUDU-2331: Allow move tool to work when uninvolved tserver is down ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/9510/5/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/9510/5/src/kudu/tools/kudu-admin-test.cc@246 PS5, Line 246: ts_flags = master_flags = { "--raft_prepare_replacement_before_eviction=true" }; > It seems it's not a direct change of this patch, but while you are at it. Done http://gerrit.cloudera.org:8080/#/c/9510/5/src/kudu/tools/kudu-admin-test.cc@278 PS5, Line 278: // To test that the move fails if any peer is down, when downTS is : // 'TabletPeer', shut down a server besides 'add' that hosts a replica. : if (downTS == DownTS::TabletPeer) { : NO_FATALS(cluster_->tablet_server_by_uuid(active_tservers.back())->Shutdown()); : } : // Regression case for KUDU-2331, where move_replica would fail if any tablet : // server is down, even if that tablet server was not involved in the move. : if (downTS == DownTS::UninvolvedTS) { : NO_FATALS(cluster_->tablet_server_by_uuid(inactive_tservers.back())->Shutdown()); : } > Nit: can be else-if, or a switch. Done http://gerrit.cloudera.org:8080/#/c/9510/5/src/kudu/tools/kudu-admin-test.cc@291 PS5, Line 291: AllowSlowTests() && downTS == DownTS::None > nit: maybe, add parenthesis for better readability? Done http://gerrit.cloudera.org:8080/#/c/9510/5/src/kudu/tools/kudu-admin-test.cc@307 PS5, Line 307: ASSERT_TRUE(s.IsRuntimeError()); > Does it make sense to narrow this down and check for some specific error me No, because RunKuduTool returns a generic "exited with status 1" message. http://gerrit.cloudera.org:8080/#/c/9510/5/src/kudu/tools/kudu-admin-test.cc@309 PS5, Line 309: SUCCEED(); > What does this do? What happens if you just returned? Nothing, just that the test has succeeded. Maybe it adds nothing alongside the return...I'll remove it. -- To view, visit http://gerrit.cloudera.org:8080/9510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id1fc60233f4f50f478da7ccb104e37df3067400c Gerrit-Change-Number: 9510 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Tue, 13 Mar 2018 06:44:25 +0000 Gerrit-HasComments: Yes
