[SPARK-7237] Many user provided closures are not actually cleaned #5787

andrewor14 · 2015-04-29T19:57:11Z

Note: ~140 lines are tests.

In a nutshell, we never cleaned closures the user provided through the following operations:

sortBy
keyBy
mapPartitions
mapPartitionsWithIndex
aggregateByKey
foldByKey
foreachAsync
one of the aliases for runJob
runApproximateJob

For more details on a reproduction and why they were not cleaned, please see SPARK-7237.

Tests should fail as of this commit because the issue hasn't been fixed yet.

Now the test added in the previous commit passes!

andrewor14 · 2015-04-29T20:02:29Z

@zsxwing @pwendell Please have a look.

SparkQA · 2015-04-29T20:38:06Z

Test build #31318 has finished for PR 5787 at commit e83699e.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

andrewor14 · 2015-04-29T21:04:40Z

Um, retest this please

SparkQA · 2015-04-29T21:41:09Z

Test build #31314 has finished for PR 5787 at commit 8ac3074.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

zsxwing · 2015-04-29T22:08:13Z

core/src/test/scala/org/apache/spark/util/ClosureCleanerSuite.scala

+  def testFilter(rdd: RDD[Int]): Unit = { rdd.filter { _ => return; true }.count() }
+  def testSortBy(rdd: RDD[Int]): Unit = { rdd.sortBy { _ => return; 1 }.count() }
+  def testKeyBy(rdd: RDD[Int]): Unit = { rdd.keyBy { _ => return; 1 }.count() }
+  def testGroupBy(rdd: RDD[Int]): Unit = { rdd.groupBy { _ => return; 1 }.count() }


No test for testGroupBy

Oops good catch. I just verified that all other test methods are called.

SparkQA · 2015-04-29T22:49:43Z

Test build #31328 has finished for PR 5787 at commit e83699e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

zsxwing · 2015-04-30T00:57:11Z

Sorry. Just deleted my wrong comments.

mapPartitionsWithContext needs to be updated too.

zsxwing · 2015-04-30T01:02:27Z

core/src/main/scala/org/apache/spark/rdd/AsyncRDDActions.scala

@@ -119,7 +119,8 @@ class AsyncRDDActions[T: ClassTag](self: RDD[T]) extends Serializable with Loggi
   * Applies a function f to each partition of this RDD.
   */
  def foreachPartitionAsync(f: Iterator[T] => Unit): FutureAction[Unit] = {
-    self.context.submitJob[T, Unit, Unit](self, f, Range(0, self.partitions.length),
+    val cleanedF = self.context.clean(f)
+    self.context.submitJob[T, Unit, Unit](self, cleanedF, Range(0, self.partitions.length),


No need to clean here. submitJob will clean f.

SparkQA · 2015-04-30T02:18:31Z

Test build #31359 has finished for PR 5787 at commit 6498f44.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

andrewor14 · 2015-05-02T01:11:20Z

@zsxwing I didn't update that one (and others like foreachWith) because it's deprecated. I think it's fine to leave those out.

SparkQA · 2015-05-02T02:51:05Z

Test build #31640 has finished for PR 5787 at commit df3caa3.

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

Conflicts: core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala

SparkQA · 2015-05-03T02:27:19Z

Test build #31675 has finished for PR 5787 at commit 7265865.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- abstract class Dialect
- class DialectException(msg: String, cause: Throwable) extends Exception(msg, cause)

andrewor14 · 2015-05-03T02:35:29Z

Anything else? Can I merge this?

andrewor14 · 2015-05-05T16:30:45Z

Alright, I'm merging this into master and 1.4, thanks everyone.

Conflicts: core/src/main/scala/org/apache/spark/rdd/RDD.scala

Note: ~140 lines are tests. In a nutshell, we never cleaned closures the user provided through the following operations: - sortBy - keyBy - mapPartitions - mapPartitionsWithIndex - aggregateByKey - foldByKey - foreachAsync - one of the aliases for runJob - runApproximateJob For more details on a reproduction and why they were not cleaned, please see [SPARK-7237](https://issues.apache.org/jira/browse/SPARK-7237). Author: Andrew Or <[email protected]> Closes #5787 from andrewor14/clean-more and squashes the following commits: 2f1f476 [Andrew Or] Merge branch 'master' of github.com:apache/spark into clean-more 7265865 [Andrew Or] Merge branch 'master' of github.com:apache/spark into clean-more df3caa3 [Andrew Or] Address comments 7a3cc80 [Andrew Or] Merge branch 'master' of github.com:apache/spark into clean-more 6498f44 [Andrew Or] Add missing test for groupBy e83699e [Andrew Or] Clean one more 8ac3074 [Andrew Or] Prevent NPE in tests when CC is used outside of an app 9ac5f9b [Andrew Or] Clean closures that are not currently cleaned 19e33b4 [Andrew Or] Add tests for all public RDD APIs that take in closures (cherry picked from commit 1fdabf8) Signed-off-by: Andrew Or <[email protected]>

AmplabJenkins · 2015-05-05T16:52:21Z

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

Note: ~140 lines are tests. In a nutshell, we never cleaned closures the user provided through the following operations: - sortBy - keyBy - mapPartitions - mapPartitionsWithIndex - aggregateByKey - foldByKey - foreachAsync - one of the aliases for runJob - runApproximateJob For more details on a reproduction and why they were not cleaned, please see [SPARK-7237](https://issues.apache.org/jira/browse/SPARK-7237). Author: Andrew Or <[email protected]> Closes apache#5787 from andrewor14/clean-more and squashes the following commits: 2f1f476 [Andrew Or] Merge branch 'master' of github.com:apache/spark into clean-more 7265865 [Andrew Or] Merge branch 'master' of github.com:apache/spark into clean-more df3caa3 [Andrew Or] Address comments 7a3cc80 [Andrew Or] Merge branch 'master' of github.com:apache/spark into clean-more 6498f44 [Andrew Or] Add missing test for groupBy e83699e [Andrew Or] Clean one more 8ac3074 [Andrew Or] Prevent NPE in tests when CC is used outside of an app 9ac5f9b [Andrew Or] Clean closures that are not currently cleaned 19e33b4 [Andrew Or] Add tests for all public RDD APIs that take in closures

Andrew Or added 3 commits April 29, 2015 12:48

Add tests for all public RDD APIs that take in closures

19e33b4

Tests should fail as of this commit because the issue hasn't been fixed yet.

Clean closures that are not currently cleaned

9ac5f9b

Now the test added in the previous commit passes!

Prevent NPE in tests when CC is used outside of an app

8ac3074

andrewor14 mentioned this pull request Apr 29, 2015

[SPARK-7120][SPARK-7121] Closure cleaner nesting + documentation + tests #5685

Closed

Clean one more

e83699e

zsxwing reviewed Apr 29, 2015
View reviewed changes

Add missing test for groupBy

6498f44

zsxwing reviewed Apr 30, 2015
View reviewed changes

Andrew Or added 2 commits May 1, 2015 18:05

Merge branch 'master' of github.com:apache/spark into clean-more

7a3cc80

Address comments

df3caa3

Merge branch 'master' of github.com:apache/spark into clean-more

7265865

Conflicts: core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala

Merge branch 'master' of github.com:apache/spark into clean-more

2f1f476

Conflicts: core/src/main/scala/org/apache/spark/rdd/RDD.scala

asfgit closed this in 1fdabf8 May 5, 2015

andrewor14 deleted the clean-more branch May 5, 2015 16:41

andrewor14 mentioned this pull request May 19, 2015

[SPARK-7741] [STREAMING] Added sparkContext.clean at missing places #6268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-7237] Many user provided closures are not actually cleaned #5787

[SPARK-7237] Many user provided closures are not actually cleaned #5787

andrewor14 commented Apr 29, 2015

andrewor14 commented Apr 29, 2015

SparkQA commented Apr 29, 2015

andrewor14 commented Apr 29, 2015

SparkQA commented Apr 29, 2015

zsxwing Apr 29, 2015

andrewor14 Apr 30, 2015

SparkQA commented Apr 29, 2015

zsxwing commented Apr 30, 2015

zsxwing Apr 30, 2015

SparkQA commented Apr 30, 2015

andrewor14 commented May 2, 2015

SparkQA commented May 2, 2015

SparkQA commented May 3, 2015

andrewor14 commented May 3, 2015

andrewor14 commented May 5, 2015

AmplabJenkins commented May 5, 2015

[SPARK-7237] Many user provided closures are not actually cleaned #5787

[SPARK-7237] Many user provided closures are not actually cleaned #5787

Conversation

andrewor14 commented Apr 29, 2015

andrewor14 commented Apr 29, 2015

SparkQA commented Apr 29, 2015

andrewor14 commented Apr 29, 2015

SparkQA commented Apr 29, 2015

zsxwing Apr 29, 2015

Choose a reason for hiding this comment

andrewor14 Apr 30, 2015

Choose a reason for hiding this comment

SparkQA commented Apr 29, 2015

zsxwing commented Apr 30, 2015

zsxwing Apr 30, 2015

Choose a reason for hiding this comment

SparkQA commented Apr 30, 2015

andrewor14 commented May 2, 2015

SparkQA commented May 2, 2015

SparkQA commented May 3, 2015

andrewor14 commented May 3, 2015

andrewor14 commented May 5, 2015

AmplabJenkins commented May 5, 2015