Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8989 )
Change subject: [raft_consensus-itest] proper fix for Test_KUDU_1735 ...................................................................... Patch Set 3: (5 comments) looks good, just some small nits http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2428 PS3, Line 2428: vector<TServerDetails*> tservers; nit: this vector isn't really needed anymore http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2430 PS3, Line 2430: ASSERT_GE(tservers.size(), 3); nit: I don't think this is really required but if so it can just be ASSERT_EQ(3 http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2432 PS3, Line 2432: tservers[0] nit: how about: tablet_servers_[cluster_->tablet_server(0)->uuid()] here and on the line below? http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2477 PS3, Line 2477: attemp typo: attempt http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2497 PS3, Line 2497: ASSERT_EVENTUALLY([&] { nit: this ASSERT_EVENTUALLY can go outside the loop -- To view, visit http://gerrit.cloudera.org:8080/8989 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibd7695f2b78da9755eb109940b40bda2bc6ec02e Gerrit-Change-Number: 8989 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Comment-Date: Fri, 12 Jan 2018 21:53:12 +0000 Gerrit-HasComments: Yes
