[SPARK-6650] [core] Stop ExecutorAllocationManager when context stops. #5311

vanzin · 2015-04-01T17:02:29Z

This fixes the thread leak. I also changed the unit test to keep track
of allocated contexts and make sure they're closed after tests are
run; this is needed since some tests use this pattern:

val sc = createContext()
doSomethingThatMayThrow()
sc.stop()

This fixes the thread leak. I also changed the unit test to keep track of allocated contexts and making sure they're closed after tests are run; this is needed since some tests use this pattern: val sc = createContext() doSomethingThatMayThrow() sc.stop()

SparkQA · 2015-04-01T17:08:24Z

Test build #29548 has started for PR 5311 at commit 9886f69.

sryza · 2015-04-01T17:24:10Z

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

  import ExecutorAllocationManager._
  import ExecutorAllocationManagerSuite._

+  private val contexts = new mutable.ListBuffer[SparkContext]()


Will this ever have more than a single element?

Yes. See test("verify min/max executors").

SparkQA · 2015-04-01T17:43:20Z

Test build #29551 has started for PR 5311 at commit cc5a744.

SparkQA · 2015-04-01T18:32:54Z

Test build #29548 has finished for PR 5311 at commit 9886f69.

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

AmplabJenkins · 2015-04-01T18:32:58Z

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

vanzin · 2015-04-01T18:42:35Z

Jenkins retest this please.

SparkQA · 2015-04-01T18:48:23Z

Test build #29554 has started for PR 5311 at commit cc5a744.

SparkQA · 2015-04-01T19:29:29Z

Test build #29551 has finished for PR 5311 at commit cc5a744.

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

AmplabJenkins · 2015-04-01T19:29:33Z

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

SparkQA · 2015-04-01T19:52:02Z

Test build #29554 has finished for PR 5311 at commit cc5a744.

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

AmplabJenkins · 2015-04-01T19:52:07Z

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

srowen · 2015-04-01T20:15:01Z

I'm not sure why the test failed, but on inspection it LGTM.

sryza · 2015-04-01T21:05:33Z

core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala

@@ -17,10 +17,12 @@

 package org.apache.spark

+import java.util.concurrent.{TimeUnit, Executors}


nit: alphabetize

sryza · 2015-04-01T21:06:05Z

One last nit. Otherwise LGTM.

SparkQA · 2015-04-01T21:23:27Z

Test build #29564 has started for PR 5311 at commit 652c73b.

SparkQA · 2015-04-01T22:46:28Z

Test build #29564 has finished for PR 5311 at commit 652c73b.

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

AmplabJenkins · 2015-04-01T22:46:33Z

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

andrewor14 · 2015-04-03T02:48:30Z

LGTM2, thanks for fixing this merging into master and 1.3.

This fixes the thread leak. I also changed the unit test to keep track of allocated contexts and make sure they're closed after tests are run; this is needed since some tests use this pattern: val sc = createContext() doSomethingThatMayThrow() sc.stop() Author: Marcelo Vanzin <vanzin@cloudera.com> Closes #5311 from vanzin/SPARK-6650 and squashes the following commits: 652c73b [Marcelo Vanzin] Nits. 5711512 [Marcelo Vanzin] More exception safety. cc5a744 [Marcelo Vanzin] Stop alloc manager before scheduler. 9886f69 [Marcelo Vanzin] [SPARK-6650] [core] Stop ExecutorAllocationManager when context stops. (cherry picked from commit 45134ec) Signed-off-by: Andrew Or <andrew@databricks.com> Conflicts: core/src/main/scala/org/apache/spark/SparkContext.scala

This fixes the thread leak. I also changed the unit test to keep track of allocated contexts and make sure they're closed after tests are run; this is needed since some tests use this pattern: val sc = createContext() doSomethingThatMayThrow() sc.stop() Author: Marcelo Vanzin <vanzin@cloudera.com> Closes apache#5311 from vanzin/SPARK-6650 and squashes the following commits: 652c73b [Marcelo Vanzin] Nits. 5711512 [Marcelo Vanzin] More exception safety. cc5a744 [Marcelo Vanzin] Stop alloc manager before scheduler. 9886f69 [Marcelo Vanzin] [SPARK-6650] [core] Stop ExecutorAllocationManager when context stops. (cherry picked from commit 45134ec) Conflicts: core/src/main/scala/org/apache/spark/SparkContext.scala

PRs Merged 1. [Internal] Add AppleAwsClientFactory for Mascot (apache#577) 2. Hive: Log new metadata location in commit (apache#4681) 3. change timeout to 120 for now (apache#661) 4. Internal: Add hive_catalog parameter to SparkCatalog (apache#670) 5. Internal: Pull catalog setting to CachedClientPool (apache#673) 6. Core: Defer reading Avro metadata until ManifestFile is read (apache#5206) 7. API: Fix ID assignment in schema merging (apache#5395) 8. AWS: S3OutputStream - failure to close should persist on subsequent close calls (apache#5311) 9. API: Allow schema updates to find fields with case-insensitivity (apache#5440) 10. Spark 3.3: Spark mergeSchema to respect Spark Case Sensitivity Configuration (apache#5441)

sryza reviewed Apr 1, 2015
View reviewed changes

Stop alloc manager before scheduler.

cc5a744

More exception safety.

5711512

sryza reviewed Apr 1, 2015
View reviewed changes

Nits.

652c73b

vanzin mentioned this pull request Apr 2, 2015

[SPARK-4194] [core] Make SparkContext initialization exception-safe. #5335

Closed

asfgit closed this in 45134ec Apr 3, 2015

vanzin deleted the SPARK-6650 branch April 3, 2015 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-6650] [core] Stop ExecutorAllocationManager when context stops. #5311

[SPARK-6650] [core] Stop ExecutorAllocationManager when context stops. #5311

vanzin commented Apr 1, 2015

SparkQA commented Apr 1, 2015

sryza Apr 1, 2015

vanzin Apr 1, 2015

SparkQA commented Apr 1, 2015

SparkQA commented Apr 1, 2015

AmplabJenkins commented Apr 1, 2015

vanzin commented Apr 1, 2015

SparkQA commented Apr 1, 2015

SparkQA commented Apr 1, 2015

AmplabJenkins commented Apr 1, 2015

SparkQA commented Apr 1, 2015

AmplabJenkins commented Apr 1, 2015

srowen commented Apr 1, 2015

sryza Apr 1, 2015

sryza commented Apr 1, 2015

SparkQA commented Apr 1, 2015

SparkQA commented Apr 1, 2015

AmplabJenkins commented Apr 1, 2015

andrewor14 commented Apr 3, 2015

		@@ -17,10 +17,12 @@

		package org.apache.spark

		import java.util.concurrent.{TimeUnit, Executors}

[SPARK-6650] [core] Stop ExecutorAllocationManager when context stops. #5311

[SPARK-6650] [core] Stop ExecutorAllocationManager when context stops. #5311

Conversation

vanzin commented Apr 1, 2015

SparkQA commented Apr 1, 2015

sryza Apr 1, 2015

Choose a reason for hiding this comment

vanzin Apr 1, 2015

Choose a reason for hiding this comment

SparkQA commented Apr 1, 2015

SparkQA commented Apr 1, 2015

AmplabJenkins commented Apr 1, 2015

vanzin commented Apr 1, 2015

SparkQA commented Apr 1, 2015

SparkQA commented Apr 1, 2015

AmplabJenkins commented Apr 1, 2015

SparkQA commented Apr 1, 2015

AmplabJenkins commented Apr 1, 2015

srowen commented Apr 1, 2015

sryza Apr 1, 2015

Choose a reason for hiding this comment

sryza commented Apr 1, 2015

SparkQA commented Apr 1, 2015

SparkQA commented Apr 1, 2015

AmplabJenkins commented Apr 1, 2015

andrewor14 commented Apr 3, 2015