You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by dongjoon-hyun <gi...@git.apache.org> on 2018/09/16 03:08:25 UTC
[GitHub] spark pull request #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky Ext...
GitHub user dongjoon-hyun opened a pull request:
https://github.com/apache/spark/pull/22432
[SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAppendOnlyMapSuite due to timeout
## What changes were proposed in this pull request?
SPARK-22713 uses `eventually` with the default timeout `150ms`. It causes flakiness because it's only executed once when GC is slow.
```scala
eventually {
System.gc()
...
}
```
**Failures**
```scala
org.scalatest.exceptions.TestFailedDueToTimeoutException:
The code passed to eventually never returned normally.
Attempted 1 times over 501.22261 milliseconds.
Last failure message: tmpIsNull was false.
```
- https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-sbt-hadoop-2.7/4916
- https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-sbt-hadoop-2.7/4906
- https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-sbt-hadoop-2.7/4907
## How was this patch tested?
Pass the Jenkins.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dongjoon-hyun/spark SPARK-22713
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22432.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #22432
----
commit 04c3f7b3c2a1b6a79d571ca2079ca6cc477027a7
Author: Dongjoon Hyun <do...@...>
Date: 2018-09-16T02:55:05Z
[SPARK-22713][CORE][FOLLOWUP] Fix flaky ExternalAppendOnlyMapSuite due to timeout
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky Ext...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22432
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22432
**[Test build #96108 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96108/testReport)** for PR 22432 at commit [`04c3f7b`](https://github.com/apache/spark/commit/04c3f7b3c2a1b6a79d571ca2079ca6cc477027a7).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22432
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96104/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22432
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22432
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96108/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22432
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky Ext...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22432#discussion_r217916510
--- Diff: core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala ---
@@ -457,7 +458,7 @@ class ExternalAppendOnlyMapSuite extends SparkFunSuite
// https://github.com/scala/scala/blob/2.13.x/test/junit/scala/tools/testing/AssertUtil.scala
// (lines 69-89)
// assert(map.currentMap == null)
- eventually {
+ eventually(timeout(5 seconds), interval(200 milliseconds)) {
--- End diff --
+1 for not overkilling it. If this is the only flaky test, let's fix this instance
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/22432
Thank you, @cloud-fan and @HyukjinKwon.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22432
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/22432
cc @eyalfa @cloud-fan
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22432
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky Ext...
Posted by eyalfa <gi...@git.apache.org>.
Github user eyalfa commented on a diff in the pull request:
https://github.com/apache/spark/pull/22432#discussion_r217902092
--- Diff: core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala ---
@@ -457,7 +458,7 @@ class ExternalAppendOnlyMapSuite extends SparkFunSuite
// https://github.com/scala/scala/blob/2.13.x/test/junit/scala/tools/testing/AssertUtil.scala
// (lines 69-89)
// assert(map.currentMap == null)
- eventually {
+ eventually(timeout(5 seconds), interval(200 milliseconds)) {
--- End diff --
@dongjoon-hyun, thanks for cleaning up my mess! :sunglasses:
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky Ext...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22432#discussion_r217902367
--- Diff: core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala ---
@@ -457,7 +458,7 @@ class ExternalAppendOnlyMapSuite extends SparkFunSuite
// https://github.com/scala/scala/blob/2.13.x/test/junit/scala/tools/testing/AssertUtil.scala
// (lines 69-89)
// assert(map.currentMap == null)
- eventually {
+ eventually(timeout(5 seconds), interval(200 milliseconds)) {
--- End diff --
This is a tiny issue annoying us. It's not a mess. :) Thank you again for your quick response.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22432
**[Test build #96104 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96104/testReport)** for PR 22432 at commit [`04c3f7b`](https://github.com/apache/spark/commit/04c3f7b3c2a1b6a79d571ca2079ca6cc477027a7).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky Ext...
Posted by eyalfa <gi...@git.apache.org>.
Github user eyalfa commented on a diff in the pull request:
https://github.com/apache/spark/pull/22432#discussion_r217901988
--- Diff: core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala ---
@@ -457,7 +458,7 @@ class ExternalAppendOnlyMapSuite extends SparkFunSuite
// https://github.com/scala/scala/blob/2.13.x/test/junit/scala/tools/testing/AssertUtil.scala
// (lines 69-89)
// assert(map.currentMap == null)
- eventually {
+ eventually(timeout(5 seconds), interval(200 milliseconds)) {
--- End diff --
I think best practice to encounter this is specifying config patience in sbt's test options.
having that said, I'v once had to 'grant' a longer timeout to a specific class so I've achieved this by overriding the `spanScalaeFactor` method
`override def spanScaleFactor: Double = super.spanScaleFactor * 3`
please notice that there's another usage of `eventually` in line 519, this one 'manually' waits 500 millis before testing which might explain why you didn't see it failing in CI, looking at it now it seems like a bad practice since `eventually` is designed to control both the timeout and the intervals between trying.
to summarize: best practice is to control this in sbt's test settings, if needed you can further control it in a specific class, in any case you have to make sure you handle all invocations of `eventually` (which is easier and less error prone by leveraging scalaTest's mechanisms).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22432
thanks, merging to master/2.4!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky Ext...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22432#discussion_r217902327
--- Diff: core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala ---
@@ -457,7 +458,7 @@ class ExternalAppendOnlyMapSuite extends SparkFunSuite
// https://github.com/scala/scala/blob/2.13.x/test/junit/scala/tools/testing/AssertUtil.scala
// (lines 69-89)
// assert(map.currentMap == null)
- eventually {
+ eventually(timeout(5 seconds), interval(200 milliseconds)) {
--- End diff --
Thank you for review, @eyalfa .
If you are looking at Spark code, you must notice that the most test cases are designed to have their suitable timeout and interval. We don't use a long global value for all test cases. That will hide potential big issues.
Also, I already knew that the other instance you mentioned, and 4 instance more on `ProcessingTimeExecutorSuite`. However, I didn't want to change it because I usually don't touch the test cases if it's not flaky.
Specifically, this instance is really serious in our Jenkins environment. The above 7 failures are only recent samples from 2 test series. We have additional 3 test series on branch `master`; maven-hadoop2.6, maven-hadoop2.7, sbt-scala-2.12. For `branch-2.4`, we have 4 more.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22432
**[Test build #96104 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96104/testReport)** for PR 22432 at commit [`04c3f7b`](https://github.com/apache/spark/commit/04c3f7b3c2a1b6a79d571ca2079ca6cc477027a7).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22432
**[Test build #96108 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96108/testReport)** for PR 22432 at commit [`04c3f7b`](https://github.com/apache/spark/commit/04c3f7b3c2a1b6a79d571ca2079ca6cc477027a7).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22432
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3137/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22432
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22432: [SPARK-22713][CORE][TEST][FOLLOWUP] Fix flaky ExternalAp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22432
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3134/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org