You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by witgo <gi...@git.apache.org> on 2015/04/17 03:58:36 UTC
[GitHub] spark pull request: [SPARK-6963][CORE]Flaky test: o.a.s.ContextCle...
GitHub user witgo opened a pull request:
https://github.com/apache/spark/pull/5548
[SPARK-6963][CORE]Flaky test: o.a.s.ContextCleanerSuite automatically cleanup checkpoint
cc @andrewor14
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/witgo/spark SPARK-6963
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/5548.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 #5548
----
commit b08b3c994902629b54808c27841335fb6ca2715d
Author: GuoQiang Li <wi...@qq.com>
Date: 2015-04-17T01:56:11Z
Flaky test: o.a.s.ContextCleanerSuite automatically cleanup checkpoint
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [WIP][SPARK-6963][CORE]Flaky test: o.a.s.Conte...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/5548#discussion_r28586982
--- Diff: core/src/test/scala/org/apache/spark/ContextCleanerSuite.scala ---
@@ -427,12 +429,17 @@ class CleanerTester(
def broadcastCleaned(broadcastId: Long): Unit = {
toBeCleanedBroadcstIds -= broadcastId
- logInfo("Broadcast" + broadcastId + " cleaned")
+ logInfo("Broadcast " + broadcastId + " cleaned")
}
def accumCleaned(accId: Long): Unit = {
logInfo("Cleaned accId " + accId + " cleaned")
}
+
+ def checkpointCleaned(rddId: Long): Unit = {
+ toBeCheckpointIds -= rddId
--- End diff --
Great, that's a good explanation. I see that this makes it effectively wait longer to proceed, by which time some necessary conditions are true, like that checkpoint cleanup has happened. Thank you for that detail, and to the limits of my knowledge, this looks good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [WIP][SPARK-6963][CORE]Flaky test: o.a.s.Conte...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/5548#discussion_r28582541
--- Diff: core/src/test/scala/org/apache/spark/ContextCleanerSuite.scala ---
@@ -245,7 +245,7 @@ class ContextCleanerSuite extends ContextCleanerSuiteBase {
assert(fs.exists(RDDCheckpointData.rddCheckpointDataPath(sc, rddId).get))
// Test that GC causes checkpoint data cleanup after dereferencing the RDD
- postGCTester = new CleanerTester(sc, Seq(rddId), Nil, Nil)
+ postGCTester = new CleanerTester(sc, Seq(rddId), Nil, Nil, Seq(rddId))
--- End diff --
How does this fix the assertion that failed? you add a new set of IDs that must now be counted down by a new callback, and you assert it ends up empty, which is good. But the assertion that failed was earlier:
```
assert(fs.exists(RDDCheckpointData.rddCheckpointDataPath(sc, rddId).get))
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [WIP][SPARK-6963][CORE]Flaky test: o.a.s.Conte...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5548#issuecomment-93951778
[Test build #30476 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30476/consoleFull) for PR 5548 at commit [`964aea7`](https://github.com/apache/spark/commit/964aea74235cfaf0150270e256eef00dbabb0b85).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [WIP][SPARK-6963][CORE]Flaky test: o.a.s.Conte...
Posted by witgo <gi...@git.apache.org>.
Github user witgo commented on a diff in the pull request:
https://github.com/apache/spark/pull/5548#discussion_r28583156
--- Diff: core/src/test/scala/org/apache/spark/ContextCleanerSuite.scala ---
@@ -245,7 +245,7 @@ class ContextCleanerSuite extends ContextCleanerSuiteBase {
assert(fs.exists(RDDCheckpointData.rddCheckpointDataPath(sc, rddId).get))
// Test that GC causes checkpoint data cleanup after dereferencing the RDD
- postGCTester = new CleanerTester(sc, Seq(rddId), Nil, Nil)
+ postGCTester = new CleanerTester(sc, Seq(rddId), Nil, Nil, Seq(rddId))
--- End diff --
The tips are more confusing.
The [jira](https://issues.apache.org/jira/browse/SPARK-6963) display the error in: [ContextCleanerSuite.scala#L252](https://github.com/witgo/spark/blob/SPARK-6963/core/src/test/scala/org/apache/spark/ContextCleanerSuite.scala#L252).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [WIP][SPARK-6963][CORE]Flaky test: o.a.s.Conte...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5548#issuecomment-93966605
[Test build #30476 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30476/consoleFull) for PR 5548 at commit [`964aea7`](https://github.com/apache/spark/commit/964aea74235cfaf0150270e256eef00dbabb0b85).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6963][CORE]Flaky test: o.a.s.ContextCle...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/5548
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [WIP][SPARK-6963][CORE]Flaky test: o.a.s.Conte...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5548#issuecomment-93966617
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30476/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6963][CORE]Flaky test: o.a.s.ContextCle...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5548#issuecomment-93877966
[Test build #30453 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30453/consoleFull) for PR 5548 at commit [`b08b3c9`](https://github.com/apache/spark/commit/b08b3c994902629b54808c27841335fb6ca2715d).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6963][CORE]Flaky test: o.a.s.ContextCle...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5548#issuecomment-93887249
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30453/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [WIP][SPARK-6963][CORE]Flaky test: o.a.s.Conte...
Posted by witgo <gi...@git.apache.org>.
Github user witgo commented on a diff in the pull request:
https://github.com/apache/spark/pull/5548#discussion_r28581367
--- Diff: core/src/test/scala/org/apache/spark/ContextCleanerSuite.scala ---
@@ -245,7 +245,7 @@ class ContextCleanerSuite extends ContextCleanerSuiteBase {
assert(fs.exists(RDDCheckpointData.rddCheckpointDataPath(sc, rddId).get))
// Test that GC causes checkpoint data cleanup after dereferencing the RDD
- postGCTester = new CleanerTester(sc, Seq(rddId), Nil, Nil)
+ postGCTester = new CleanerTester(sc, Seq(rddId), Nil, Nil, Seq(rddId))
--- End diff --
@srowen This is just a test code bug. `new CleanerTester(sc, Seq(rddId), Nil, Nil)` This code is only to ensure that the RDD is cleaned, but checkpoint. rdd and checkpoint almost simultaneously be cleaned, But there are exceptions, depending on the GC. The `checkpointCleaned ` ensure the checkpoint is cleaned.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6963][CORE]Flaky test: o.a.s.ContextCle...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5548#issuecomment-93887247
[Test build #30453 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30453/consoleFull) for PR 5548 at commit [`b08b3c9`](https://github.com/apache/spark/commit/b08b3c994902629b54808c27841335fb6ca2715d).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6963][CORE]Flaky test: o.a.s.ContextCle...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/5548#issuecomment-93941009
I'd like to start pushing back on PRs without explanation, here or in the JIRA. Please add a short explanation of why this is the right change and what the problem is. Otherwise I think we need to start rejecting these.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [WIP][SPARK-6963][CORE]Flaky test: o.a.s.Conte...
Posted by witgo <gi...@git.apache.org>.
Github user witgo commented on a diff in the pull request:
https://github.com/apache/spark/pull/5548#discussion_r28584757
--- Diff: core/src/test/scala/org/apache/spark/ContextCleanerSuite.scala ---
@@ -427,12 +429,17 @@ class CleanerTester(
def broadcastCleaned(broadcastId: Long): Unit = {
toBeCleanedBroadcstIds -= broadcastId
- logInfo("Broadcast" + broadcastId + " cleaned")
+ logInfo("Broadcast " + broadcastId + " cleaned")
}
def accumCleaned(accId: Long): Unit = {
logInfo("Cleaned accId " + accId + " cleaned")
}
+
+ def checkpointCleaned(rddId: Long): Unit = {
+ toBeCheckpointIds -= rddId
--- End diff --
@srowen When the checkpoint is cleaned, `checkpointCleaned ` is called asynchronous, `isAllCleanedUp` returns true, and then `postGCTester.assertCleanup()` return.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [WIP][SPARK-6963][CORE]Flaky test: o.a.s.Conte...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/5548#discussion_r28584216
--- Diff: core/src/test/scala/org/apache/spark/ContextCleanerSuite.scala ---
@@ -245,7 +245,7 @@ class ContextCleanerSuite extends ContextCleanerSuiteBase {
assert(fs.exists(RDDCheckpointData.rddCheckpointDataPath(sc, rddId).get))
// Test that GC causes checkpoint data cleanup after dereferencing the RDD
- postGCTester = new CleanerTester(sc, Seq(rddId), Nil, Nil)
+ postGCTester = new CleanerTester(sc, Seq(rddId), Nil, Nil, Seq(rddId))
--- End diff --
Ah right, it is the following one. Still, how does this affect whether the checkpoint files exist?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org