[WIP] Remove many uses of Thread.sleep() from streaming tests by JoshRosen · Pull Request #3687 · apache/spark

JoshRosen · 2014-12-13T01:41:29Z

Update: we have decided to split this into a series of smaller PRs: #3801 and #3832.

This is a work-in-progress PR towards removing many of the Thread.sleep() calls from Spark Streaming's test suite, since I think that these calls make these tests race-prone and flaky.

Running list of the JIRAs that this patch addresses / intends to address:

SPARK-4835: "Streaming saveAs*HadoopFiles() methods may throw FileAlreadyExistsException during checkpoint recovery"
SPARK-1600: "Flaky "recovery with file input stream" test in streaming.CheckpointSuite"

TODOS:

Fix SPARK-1600 by removing the Thread.sleep calls from the recovery with file input stream test. There are several uses of Thread.sleep here which seem to serve several different purposes, so this will require some care.
Remove the sleep calls in InputStreamSuite. Most of these cases look simple to replace with the new StreamingTestWaiter, but the testFileStream case looks like it could be tricky. Going to defer removing the sleeps from multi-thread receiver since that looks like a bit of work and we can do it later if we observe flakiness.
Investigate the uses in ReceiverSuite. There are a few Thread.sleep(0) cases which seem confusing; if they're still necessary, they should be commented.
Might be able to remove one or two low-hanging fruit calls in MasterFailureTest, since these seem to be blocking on events like the StreamingContext starting and we have waiters for this.
Determine whether the fix for SPARK-4835 should be done differently (see discussion on JIRA).

…est" This reverts commit f8f6c93. Conflicts: streaming/src/test/scala/org/apache/spark/streaming/InputStreamsSuite.scala

SparkQA · 2014-12-13T01:42:30Z

Test build #24424 has started for PR 3687 at commit ad0056b.

This patch merges cleanly.

JoshRosen · 2014-12-13T01:55:50Z

I expect this to fail due to https://issues.apache.org/jira/browse/SPARK-4835.

SparkQA · 2014-12-13T02:45:30Z

Test build #24424 has finished for PR 3687 at commit ad0056b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class StreamingTestWaiter(ssc: StreamingContext)

AmplabJenkins · 2014-12-13T02:45:35Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24424/
Test FAILed.

JoshRosen · 2014-12-13T05:15:44Z

/cc @tdas. This is a work-in-progress towards removing most of the Thread.sleep() calls. I'm making slow-but-steady progress; would love your feedback + any suggestions for other types of test restructuring / cleanup to improve stability.

SparkQA · 2014-12-13T08:27:27Z

Test build #24434 has started for PR 3687 at commit 3db335f.

This patch merges cleanly.

…n class.

SparkQA · 2014-12-13T09:02:27Z

Test build #24435 has started for PR 3687 at commit 12635b4.

This patch merges cleanly.

SparkQA · 2014-12-13T09:31:20Z

Test build #24434 has finished for PR 3687 at commit 3db335f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-12-13T09:31:24Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24434/
Test FAILed.

tdas · 2014-12-13T09:58:59Z

@JoshRosen This is a wonderful and much-required refactoring. I havent been able to see it in detail yet, will do when I reach home.

SparkQA · 2014-12-13T10:06:11Z

Test build #24435 has finished for PR 3687 at commit 12635b4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-12-13T10:06:14Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24435/
Test FAILed.

tdas · 2014-12-13T10:09:04Z

I am a little uneasy with this approach because its not clean. Ideally nothing in Spark should refer to the requirements of higher level libraries like Spark Streaming.

JoshRosen · 2014-12-13T21:38:50Z

This latest round of test failures was due to the flaky WriteAheadLogBackedBlockRDDSuite tests (SPARK-4826). I'm going to work on investigating that test now, but I'll probably fix that in a separate PR since I think that the root cause there is not Thread.sleep calls (and that fix needs to be targeted differently for backports, since that feature is new in 1.2.0).

JoshRosen · 2014-12-16T21:54:30Z

This latest run failed three tests:

org.apache.spark.streaming.CheckpointSuite.recovery with file input stream
org.apache.spark.streaming.FailureSuite.multiple failures with map
org.apache.spark.streaming.FailureSuite.multiple failures with updateStateByKey

Here are the failed assertions:

https://github.com/JoshRosen/spark/blob/streaming-test-flakiness/streaming/src/test/scala/org/apache/spark/streaming/MasterFailureTest.scala#L84, which, unfortunately, used == instead of === so I don't get a useful error message.
Similarly, https://github.com/JoshRosen/spark/blob/streaming-test-flakiness/streaming/src/test/scala/org/apache/spark/streaming/MasterFailureTest.scala#L117
https://github.com/JoshRosen/spark/blob/streaming-test-flakiness/streaming/src/test/scala/org/apache/spark/streaming/CheckpointSuite.scala#L350

It's possible that this could have been a side-effect of the Charsets.UTF_8 change, but I don't see how that could have caused problems. I'll investigate.

JoshRosen · 2014-12-17T01:49:37Z

It turns out that my current use of file.setLastModified is brittle since the underlying filesystem may only support one-second resolution:

 * <p> All platforms support file-modification times to the nearest second,
* but some provide more precision. The argument will be truncated to fit
* the supported precision. If the operation succeeds and no intervening
* operations on the file take place, then the next invocation of the
* <code>{@link #lastModified}</code> method will return the (possibly
* truncated) <code>time</code> argument that was passed to this method.

I wish there was a better way to mock the filesystem in these tests. In the meantime, though, I could stick with batch intervals that are multiples of seconds (since we only need to do this in two tests).

SparkQA · 2014-12-17T02:12:36Z

Test build #24523 has started for PR 3687 at commit 520bade.

This patch merges cleanly.

JoshRosen · 2014-12-17T02:21:03Z

Alright, I think I fixed up a few problems in my first version of the SPARK-1600 fix, so hopefully it works now.

For those who are interested in the details:

We had a "time-travel" issue when restarting from the checkpoint because I didn't copy over the old ManualClock value to the new StreamingContext.
My modified FileInputDStream didn't handle clock properly during recovery (the field shouldn't have been @transient, so I made it into a def instead).

SparkQA · 2014-12-17T02:22:29Z

Test build #24524 has started for PR 3687 at commit 1304776.

This patch merges cleanly.

JoshRosen · 2014-12-17T02:25:13Z

I removed this and the tests seem to pass, but now I'm wondering whether I've inadvertently introduced a new source of flakiness...

In this attempt to remove all sleep (that is, removing all waiting related logic), it is a good idea that we remove this and fix all tests that uses set this to true.

SparkQA · 2014-12-17T03:36:08Z

Test build #24523 has finished for PR 3687 at commit 520bade.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-12-17T03:36:11Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24523/
Test PASSed.

SparkQA · 2014-12-17T03:43:45Z

Test build #24524 has finished for PR 3687 at commit 1304776.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-12-17T03:43:49Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24524/
Test PASSed.

tdas · 2014-12-25T01:28:07Z

I approve this change. But probably should be a different PR that just touches this input stream and its tests.

tdas · 2014-12-25T01:58:24Z

At a high-level i think, lets spilt this PR into two PRs

Fixing saveAsHadoopFiles
Fixing ManualClock and its uses
Removing thread sleeps

This would allow the discussion of one to proceed independently of the other. Frankly, given the current state in this PR, (3) and (2) is closer to being merged than (1).

JoshRosen · 2014-12-25T07:15:38Z

EDIT: see following comment instead

I agree that it's a good idea to split this up.

For starters, I'm going to try splitting off only the fix for the FileInputDStream test (SPARK-1600), since that's one of the flakiest tests, has some somewhat-unique code changes (the file modification timestamp stuff) and should be a relatively small PR to review by itself. I'll introduce the StreamingTestWaiter class in that PR. Once we're done reviewing and merging that, I'll move onto a PR to clean up all of the remaining uses of Thread.sleep(). Some of those uses have not led to flaky tests, though, so I think that splitting the change up and prioritizing based on the tests that are known to be flaky will be a good way to reduce the review burden here.

Let's chat offline about the saveAsHadoopFiles fix.

JoshRosen · 2014-12-25T08:39:38Z

Just realized that my last comment was a bit confusing, since SPARK-1600 is not related to the FileInputStream ManualClock fix. I'll file a new improvement JIRA to cover replacing our uses of SystemClock in tests.

JoshRosen · 2015-01-07T19:47:49Z

I'm going to close this for now; I'll open separate, smaller PRs for the remaining test cleanup.

JoshRosen added 5 commits December 12, 2014 15:15

Add StreamingTestWaiter class
303e828

Remove (all but one) sleep calls from “socket input stream” test
f8f6c93

Refactor several tests to use verifyOutput()
6ce0681

Revert "Remove (all but one) sleep calls from “socket input stream” t…
5c31b8a
…est" This reverts commit f8f6c93. Conflicts: streaming/src/test/scala/org/apache/spark/streaming/InputStreamsSuite.scala

WIP towards improved CheckpointSuite sleep logic.
ad0056b

JoshRosen mentioned this pull request Dec 13, 2014
[SPARK-3431] [WIP] Parallelize Scala/Java test execution #3564
Closed

JoshRosen changed the title ~~Remove many uses of Thread.sleep() from streaming tests~~[WIP] Remove many uses of Thread.sleep() from streaming testsDec 13, 2014

[SPARK-4835] Disable OutputSpec validation for streaming jobs.
3db335f

JoshRosen added 2 commits December 13, 2014 00:50

Remove several (but not all) sleep() calls in InputStreamSuite.
b245217

Remove sleep() in runStreamsWithPartitions(); use streaming’s Duratio…
12635b4
…n class.

tdas reviewed Dec 13, 2014
View reviewed changes

scwf mentioned this pull request Dec 15, 2014
[Hot Fix][Streaming] Fix WriteAheadLogBackedBlockRDDSuite #3701
Closed

JoshRosen added 3 commits December 15, 2014 23:50

Fix indentation
1a0fcb9

Refactor tests to use Charsets.UTF_8 (seeapache#2781).
c81a477

Modify FileInputDStream to use Clock class.
abf5050

JoshRosen added 4 commits December 16, 2014 14:29

Use Scalatest Assertions === in MasterFailureTest.
9c939d9

Add ability to block on batch start events.
590f006

WIP towards SPARK-1600
c9c477f

Synchronize currentTime in ManualClock
63162b2

First passing version of the refactored SPARK-1600 test.
520bade

Remove debugging line
1304776

JoshRosen reviewed Dec 17, 2014
View reviewed changes

JoshRosen mentioned this pull request Dec 17, 2014
[SPARK-4229] Create hadoop configuration in a consistent way #3543
Closed

tdas reviewed Dec 25, 2014
View reviewed changes

JoshRosen mentioned this pull request Dec 25, 2014
[SPARK-1600] Refactor FileInputStream tests to remove Thread.sleep() calls and SystemClock usage #3801
Closed

JoshRosen closed this Jan 7, 2015

Uh oh!

Conversation

JoshRosen commented Dec 13, 2014

Uh oh!

SparkQA commented Dec 13, 2014

Uh oh!

JoshRosen commented Dec 13, 2014

Uh oh!

SparkQA commented Dec 13, 2014

Uh oh!

AmplabJenkins commented Dec 13, 2014

Uh oh!

JoshRosen commented Dec 13, 2014

Uh oh!

SparkQA commented Dec 13, 2014

Uh oh!

SparkQA commented Dec 13, 2014

Uh oh!

SparkQA commented Dec 13, 2014

Uh oh!

AmplabJenkins commented Dec 13, 2014

Uh oh!

tdas commented Dec 13, 2014

Uh oh!

SparkQA commented Dec 13, 2014

Uh oh!

AmplabJenkins commented Dec 13, 2014

Uh oh!

tdasDec 13, 2014

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented Dec 13, 2014

Uh oh!

JoshRosen commented Dec 16, 2014

Uh oh!

JoshRosen commented Dec 17, 2014

Uh oh!

SparkQA commented Dec 17, 2014

Uh oh!

JoshRosen commented Dec 17, 2014

Uh oh!

SparkQA commented Dec 17, 2014

Uh oh!

JoshRosenDec 17, 2014

Choose a reason for hiding this comment

Uh oh!

tdasDec 25, 2014

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 17, 2014

Uh oh!

AmplabJenkins commented Dec 17, 2014

Uh oh!

SparkQA commented Dec 17, 2014

Uh oh!

AmplabJenkins commented Dec 17, 2014

Uh oh!

tdasDec 25, 2014

Choose a reason for hiding this comment

Uh oh!

tdas commented Dec 25, 2014

Uh oh!

JoshRosen commented Dec 25, 2014

Uh oh!

JoshRosen commented Dec 25, 2014

Uh oh!

JoshRosen commented Jan 7, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants