[SPARK-4236] Cleanup removed applications' files in shuffle service #3126

aarondav · 2014-11-06T01:16:40Z

This relies on a hook from whoever is hosting the shuffle service to invoke removeApplication() when the application is completed. Once invoked, we will clean up all the executors' shuffle directories we know about.

aarondav · 2014-11-06T01:18:06Z

network/shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockManager.java

@@ -95,6 +111,56 @@ public ManagedBuffer getBlockData(String appId, String execId, String blockId) {
  }

  /**


The following 32 lines are the only material change in this entire PR. The rest is utility functions imported from core Utils, Java cruft around lambdas and classes, and updating/adding tests.

SparkQA · 2014-11-06T01:20:15Z

Test build #22972 has started for PR 3126 at commit 0028241.

This patch merges cleanly.

SparkQA · 2014-11-06T01:35:02Z

Test build #22973 has started for PR 3126 at commit 382c97d.

This patch merges cleanly.

aarondav · 2014-11-06T01:35:32Z

@andrewor14 @rxin PTAL. For the YARN auxiliary service, I think it is sufficient just to just call ExternalShuffleBlockHandler#applicationRemoved(appId) at the appropriate time.

SparkQA · 2014-11-06T02:45:54Z

Test build #22972 has finished for PR 3126 at commit 0028241.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class AllocatedBlocks(streamIdToAllocatedBlocks: Map[Int, Seq[ReceivedBlockInfo]])
- class ReceiverTracker(ssc: StreamingContext, skipReceiverLaunch: Boolean = false) extends Logging

AmplabJenkins · 2014-11-06T02:45:57Z

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

SparkQA · 2014-11-06T03:00:34Z

Test build #22973 has finished for PR 3126 at commit 382c97d.

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

AmplabJenkins · 2014-11-06T03:00:37Z

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

andrewor14 · 2014-11-06T20:06:37Z

Hey was this already merged? I don't see it showing up on ASF.

aarondav · 2014-11-06T20:10:52Z

Whoops, accidentally deleted it locally.

SparkQA · 2014-11-06T20:14:50Z

Test build #23015 has started for PR 3126 at commit 382c97d.

This patch merges cleanly.

andrewor14 · 2014-11-06T21:23:45Z

core/src/test/scala/org/apache/spark/ExternalShuffleServiceSuite.scala

-    rpcHandler.clearRegisteredExecutors()
+    // Invalidate the registered executors, disallowing access to their shuffle blocks (without
+    // deleting the actual shuffle files, so we could access them without the shuffle service).
+    rpcHandler.applicationRemoved(sc.conf.getAppId, false /* cleanupLocalDirs */)


can you just do cleanupLocalDirs = false? Or can we not because this is a Java method even though we're using it in Scala

Right, not possible if the method is written in Java, unfortunately.

SparkQA · 2014-11-06T21:46:59Z

Test build #23015 has finished for PR 3126 at commit 382c97d.

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

AmplabJenkins · 2014-11-06T21:47:02Z

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

aarondav · 2014-11-06T23:21:55Z

Comments addressed.

SparkQA · 2014-11-06T23:24:56Z

Test build #23026 has started for PR 3126 at commit a1f73ab.

This patch merges cleanly.

SparkQA · 2014-11-07T00:58:45Z

Test build #23026 has finished for PR 3126 at commit a1f73ab.

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

AmplabJenkins · 2014-11-07T00:58:48Z

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

andrewor14 · 2014-11-07T01:38:00Z

LGTM. Hey by the way is there anything we need to change about the way ContextCleaner cleans up shuffle files now when external shuffle service is enabled? The only difference in functionality here is that we will not clean shuffle files based on scope anymore (we currently go through executors but they're now dead). This could be a problem for long-running contexts. Maybe we should add some extra TTL-based cleanup and default the timeout to a week or something in SPARK-4287.

Also I think this conflicts with your other patch which I just merged.

This relies on a hook from whoever is hosting the shuffle service to invoke removeApplication() when the application is completed. Once invoked, we will clean up all the executors' shuffle directories we know about.

aarondav · 2014-11-07T02:12:36Z

It was you -- JavaUtils changed. Updated.

SparkQA · 2014-11-07T02:15:16Z

Test build #23032 has started for PR 3126 at commit 33a64a9.

This patch merges cleanly.

andrewor14 · 2014-11-07T02:31:34Z

Weird, we both only added code to JavaUtils. I would think that it doesn't need to conflict there.

SparkQA · 2014-11-07T03:43:19Z

Test build #23032 has finished for PR 3126 at commit 33a64a9.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class StandaloneWorkerShuffleService(sparkConf: SparkConf, securityManager: SecurityManager)

AmplabJenkins · 2014-11-07T03:43:23Z

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

andrewor14 · 2014-11-07T03:53:55Z

Ok merging into master and 1.2

This relies on a hook from whoever is hosting the shuffle service to invoke removeApplication() when the application is completed. Once invoked, we will clean up all the executors' shuffle directories we know about. Author: Aaron Davidson <aaron@databricks.com> Closes #3126 from aarondav/cleanup and squashes the following commits: 33a64a9 [Aaron Davidson] Missing brace e6e428f [Aaron Davidson] Address comments 16a0d27 [Aaron Davidson] Cleanup e4df3e7 [Aaron Davidson] [SPARK-4236] Cleanup removed applications' files in shuffle service (cherry picked from commit 48a19a6) Signed-off-by: Andrew Or <andrew@databricks.com>

aarondav reviewed Nov 6, 2014
View reviewed changes

aarondav closed this Nov 6, 2014

aarondav deleted the cleanup branch November 6, 2014 18:54

aarondav reopened this Nov 6, 2014

andrewor14 reviewed Nov 6, 2014
View reviewed changes

aarondav added 3 commits November 6, 2014 18:10

[SPARK-4236] Cleanup removed applications' files in shuffle service

e4df3e7

This relies on a hook from whoever is hosting the shuffle service to invoke removeApplication() when the application is completed. Once invoked, we will clean up all the executors' shuffle directories we know about.

Cleanup

16a0d27

Address comments

e6e428f

aarondav force-pushed the cleanup branch from a1f73ab to e6e428f Compare November 7, 2014 02:10

Missing brace

33a64a9

asfgit closed this in 48a19a6 Nov 7, 2014

		@@ -95,6 +111,56 @@ public ManagedBuffer getBlockData(String appId, String execId, String blockId) {
		}

		/**

[SPARK-4236] Cleanup removed applications' files in shuffle service #3126

[SPARK-4236] Cleanup removed applications' files in shuffle service #3126

Uh oh!

Conversation

aarondav commented Nov 6, 2014

Uh oh!

aarondav Nov 6, 2014

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

aarondav commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

AmplabJenkins commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

AmplabJenkins commented Nov 6, 2014

Uh oh!

andrewor14 commented Nov 6, 2014

Uh oh!

aarondav commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

andrewor14 Nov 6, 2014

Choose a reason for hiding this comment

Uh oh!

aarondav Nov 6, 2014

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

AmplabJenkins commented Nov 6, 2014

Uh oh!

aarondav commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 7, 2014

Uh oh!

AmplabJenkins commented Nov 7, 2014

Uh oh!

andrewor14 commented Nov 7, 2014

Uh oh!

aarondav commented Nov 7, 2014

Uh oh!

SparkQA commented Nov 7, 2014

Uh oh!

andrewor14 commented Nov 7, 2014

Uh oh!

SparkQA commented Nov 7, 2014

Uh oh!

AmplabJenkins commented Nov 7, 2014

Uh oh!

andrewor14 commented Nov 7, 2014

Uh oh!

Uh oh!