Fix incorrect test parallelWritesWithoutTransaction. #829
Conversation
| public void shouldReceiveOneNotificationWithAllAffectedTablesInTransactionWithMultipleThreads() throws InterruptedException { | ||
| final String table1 = "test_table1"; | ||
| final String table2 = "test_table2"; | ||
|
|
||
| final int numberOfThreads = 100; | ||
| final int numberOfThreads = 50; |
There was a problem hiding this comment.
100 might be a problem for Travis since it has limited amount of resources
| @@ -239,14 +246,15 @@ public void run() { | |||
| // Steady! | |||
| startAllThreadsLock.countDown(); // Go! | |||
|
|
|||
| assertThat(allThreadsFinishedLock.await(20, SECONDS)).isTrue(); | |||
| assertThat(allThreadsFinishedLock.await(25, SECONDS)).isTrue(); | |||
There was a problem hiding this comment.
Doesn't really matter, but since 30 seconds is now global limit for test duration, increasing this closer to 30 kinda makes sense :)
| @@ -197,11 +203,12 @@ public void run() { | |||
| } | |||
|
|
|||
| @Test | |||
| @Repeat(times = 20) | |||
There was a problem hiding this comment.
I've seen this test failed on #827 PR build, but I don't think I ever saw it fail before, so I just added @Repeat to make sure it's good, I've read it carefully and still think it's a good test as is :)
| final int numberOfParallelWorkers = 50; | ||
| @Repeat(times = 20) | ||
| public void parallelPutWithoutGlobalTransaction() { | ||
| final int numberOfParallelPuts = 50; |
There was a problem hiding this comment.
Just rename :)
|
Ah, didn't update |
Codecov Report
@@ Coverage Diff @@
## master #829 +/- ##
==========================================
+ Coverage 97.39% 97.41% +0.01%
==========================================
Files 89 91 +2
Lines 2692 2707 +15
Branches 302 305 +3
==========================================
+ Hits 2622 2637 +15
Misses 38 38
Partials 32 32
Continue to review full report at Codecov.
|
|
@nikitin-da I'm merging this to unblock other PRs, please feel free to comment, I'll open separate PR to fix your comments if needed! @geralt-encore thanks for review! |
| import org.junit.runner.Description; | ||
| import org.junit.runners.model.Statement; | ||
|
|
||
| public class RepeatRule implements TestRule { |
|
@artem-zinnatullin great work man! |
Fixes #826.
TL;TR: test was incorrect, because default PutResolver uses short-term transaction thus StorIO has all rights to merge notifications, so sometimes we've seen flakiness due to notifications merge.
CI was freezing because we were awaiting for wrong amount of notifications without timeout, this PR adds timeout to all tests and
RepeatRuleto make sure our concurrent tests are good.Thanks a lot @nikitin-da for his work in #827, those logs really helped to figure out what was wrong!