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 <aser...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 14 Feb 2018 02:19:16 +0000
Gerrit-HasComments: Yes

Reply via email to