Alexey Serbin has posted comments on this change. Change subject: Add integration tests for duplicate keys ......................................................................
Patch Set 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/5349/7/src/kudu/integration-tests/cluster_verifier.cc File src/kudu/integration-tests/cluster_verifier.cc: PS7, Line 126: CHECK_OK(scanner.SetReadMode(client::KuduScanner::READ_AT_SNAPSHOT)); : CHECK_OK(scanner.SetFaultTolerant()); : CHECK_OK(scanner.SetTimeoutMillis(5000)); : CHECK_OK(scanner.SetProjectedColumns(vector<string>())); Is it so crucial to abort if something goes non-OK among those? Looking at that without knowing much of context here, I those non-OK returns would not break consistency, so I would expect that simple RETURN_NOT_OK() or RETURN_NOT_OK_PREPEND() would work here as well. PS7, Line 129: vector<string>() nit: consider using '{}' instead of 'vector<string>()' for brevity. http://gerrit.cloudera.org:8080/#/c/5349/7/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: PS7, Line 359: static const bool WITH_NOTIFICATION_LATENCY = true; : static const bool WITHOUT_NOTIFICATION_LATENCY = false; Are these needed in the new code? PS7, Line 870: kCrashesToCause nit: since it's not static or global, consider renaming into 'crashes_to_cause' PS7, Line 907: CHECK_OK nit: ASSERT_OK() ? PS7, Line 947: 3) Is batch of size 3 is enough to get higher chance of collision? I wonder what would happen if this set to, say, 50. PS7, Line 954: master_flags It seems this stays empty, consider removing this variable and using '{}' for corresponding argument. PS7, Line 959: ts_flags.push_back("--raft_heartbeat_interval_ms=5"); : #else : ts_flags.push_back("--raft_heartbeat_interval_ms=1"); : #endif : ts_flags.push_back("--leader_failure_monitor_check_mean_ms=1"); : ts_flags.push_back("--leader_failure_monitor_check_stddev_ms=1"); : ts_flags.push_back("--never_fsync"); nit: consider replacing the construction of the ts_flags via push_back() with list initializer instead: vector<string> ts_flags = { "--raft_heartbeat_interval_ms=5", ... }; PS7, Line 968: master_flags As described above: replace with {}. PS7, Line 1009: TEST_F(RaftConsensusITest, TestChurnyElections) { : const int kNumWrites = AllowSlowTests() ? 10000 : 1000; : CreateClusterForChurnyElectionsTests({}); : TestWorkload workload(cluster_.get()); : workload.set_write_batch_size(1); : DoTestChurnyElections(&workload, kNumWrites); : } : : // The same test, except inject artificial latency when propagating notifications : // from the queue back to consensus. This previously reproduced bugs like KUDU-1078 which : // normally only appear under high load. : TEST_F(RaftConsensusITest, TestChurnyElections_WithNotificationLatency) { : CreateClusterForChurnyElectionsTests({"--consensus_inject_latency_ms_in_notifications=50"}); : const int kNumWrites = AllowSlowTests() ? 10000 : 1000; : TestWorkload workload(cluster_.get()); : workload.set_write_batch_size(1); : DoTestChurnyElections(&workload, kNumWrites); : } : : // The same as TestChurnyElections except insert many duplicated rows. : // This emulates cases where there are many duplicate keys which, due to two phase : // locking, may cause deadlocks and other anomalies that cannot be observed when : // keys are unique. : TEST_F(RaftConsensusITest, TestChurnyElections_WithDuplicateKeys) { : CreateClusterForChurnyElectionsTests({}); : const int kNumWrites = AllowSlowTests() ? 10000 : 1000; : TestWorkload workload(cluster_.get()); : workload.set_write_pattern(TestWorkload::INSERT_WITH_MANY_DUP_KEYS); : // Increase the number of rows per batch to get a higher chance of key collision. : workload.set_write_batch_size(3); : DoTestChurnyElections(&workload, kNumWrites); These look like candidates for extracting a common function and then re-using it with different set of parameters correspondingly. -- To view, visit http://gerrit.cloudera.org:8080/5349 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6d018281d412ae034bd7b70c8311077a52b2795d Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
