Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9255 )
Change subject: KUDU-2274 [itest] stress test for replica replacement ...................................................................... Patch Set 4: (16 comments) http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc File src/kudu/integration-tests/raft_consensus_stress-itest.cc: http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@66 PS3, Line 66: 2 magic number, can we add something like /* ERROR */ or ideally use the constant from glog if possible? Also an explanation as to why we chose this as the default would be helpful for posterity. http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@79 PS3, Line 79: 2 20 for ASAN and 5 for everybody else? http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@113 PS3, Line 113: RaftConsensusITestBase Since we are not using tablet_id_ etc, can we instead use ExternalMiniClusterITestBase as the base class? http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@117 PS3, Line 117: being nit: s/being/are/ http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@122 PS3, Line 122: this test is not fit to run under TSAN nit: how about: this test is not reliable under TSAN; skipping test http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@170 PS3, Line 170: 2 kNumReplicas ? http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@175 PS3, Line 175: TODO(aserbin) TODO(KUDU-1188) http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@212 PS3, Line 212: if (do_pauses) { this loop will run hot if do_pauses is set to false http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@241 PS3, Line 241: do_elections = false; : do_pauses = false; are these lines needed? http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@265 PS3, Line 265: 0 false http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@266 PS3, Line 266: 0 false http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@276 PS3, Line 276: 1 true http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@277 PS3, Line 277: 1 true http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@284 PS3, Line 284: do_elections = false; : do_pauses = false; remove these lines? http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@293 PS3, Line 293: onst auto& s Consider saving this in a Status last_ksck_status defined next to ksck_failures_in_a_row (outside the loop) and saved on line 273, since it would be confusing to print a success message on failure if the final attempt succeeds. http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@305 PS3, Line 305: //NO_FATALS(v.CheckRowCount(workload.table_name(), Doesn't CheckRowCount() retry? Leader leases won't help here, I don't think. If we allow writes to time out, then we can actually end up writing more rows than got acknowledged. Since we are using workload.set_timeout_allowed(true), we should be using (ClusterVerifier::AT_LEAST, workload.rows_inserted()), not ClusterVerifier::EXACTLY. -- To view, visit http://gerrit.cloudera.org:8080/9255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I036a882f7e9132a5c26013227c50c0699b59ed6e Gerrit-Change-Number: 9255 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 14 Feb 2018 02:19:16 +0000 Gerrit-HasComments: Yes
