Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9255 )

Change subject: KUDU-2274 [itest] stress test for replica replacement
......................................................................


Patch Set 5:

(15 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: t
> magic number, can we add something like /* ERROR */ or ideally use the cons
Done


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@79
PS3, Line 79: "
> 20 for ASAN and 5 for everybody else?
Yep, that's the ultimate typo.  It should be inversed/swapped.


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@117
PS3, Line 117:  int
> nit: s/being/are/
Done


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@122
PS3, Line 122: ShortTimeout = MonoDelta::FromSeconds(
> nit: how about:
well, as it turned out, after fixing the ultimate typo it works for TSAN even 
without patch for TestWorkload.


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@170
PS3, Line 170:
> kNumReplicas ?
By my understanding (after looking into catalog_manager.cc), 
FLAGS_max_create_tablets_per_ts limits the number of partitions/tablets, not 
the number of total replicas.  My original idea was to have 
FLAGS_test_num_tablets_per_server as a proxy for 
FLAGS_max_create_tablets_per_ts.

However, I updated the signature and now the test speaks in number of replicas 
per tablet server: I agree it's more convenient.


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@175
PS3, Line 175: le (workload.
> TODO(KUDU-1188)
Done


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@212
PS3, Line 212:   const auto ts_
> this loop will run hot if do_pauses is set to false
Right.  I moved SleepFor(MonoDelta::FromMilliseconds(250)) out of the if() 
closure.


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@241
PS3, Line 241:     workload.StopAndJoin();
             :     rows_inserted = wo
> are these lines needed?
Yep, now I think we can get rid of them safely.  Maybe, in one of the former 
revisions they were useful.


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@265
PS3, Line 265:
> false
Done


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@266
PS3, Line 266: e
> false
Done


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@276
PS3, Line 276: T
> true
Done


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@277
PS3, Line 277: r
> true
Done


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@284
PS3, Line 284:       FAIL() << Substitute("$0: tablet replicas haven't 
converged", s.ToString());
             :     }
> remove these lines?
Done


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@293
PS3, Line 293: FATALS(v.Che
> Consider saving this in a Status last_ksck_status defined next to ksck_fail
Good catch.  It seems like just another lost update due to multiple revisions 
of this test.  I corrected the code as necessary.


http://gerrit.cloudera.org:8080/#/c/9255/3/src/kudu/integration-tests/raft_consensus_stress-itest.cc@305
PS3, Line 305:
> Doesn't CheckRowCount() retry? Leader leases won't help here, I don't think
Indeed, thank you for pointing at the right direction.  I thought the message 
was about reading less than expected, but as it turned out it was about reading 
more than expected.



--
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: 5
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 06:56:13 +0000
Gerrit-HasComments: Yes

Reply via email to