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