Alexey Serbin 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: (3 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. To make this piece independent on what replica scheme is used by default, consider replacing this conditional piece with ..._flags = { Substitute("--raft_prepare_replacement_before_eviction=$0", (enable_kudu_1097 == Kudu1097::Enable)) }; 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? 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 message? -- 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: Mon, 12 Mar 2018 21:21:16 +0000 Gerrit-HasComments: Yes
