[SPARK-2645] [Core] Allow SparkEnv.stop() to be called multiple times without side effects. #6973

rekhajoshm · 2015-06-24T04:43:41Z

Fix for SparkContext stop behavior - Allow sc.stop() to be called multiple times without side effects.

Pulling functionality from apache spark

pull latest from apache spark

Pulling functionality from apache spark

sarutak · 2015-06-24T05:51:18Z

Hi @rekhajoshm , could you explain details more in the description field?

srowen · 2015-06-24T08:32:02Z

Yeah, this might be a good change (stop() should be idempotent right?) but I'm not 100% clear how it connects to the JIRA. Also, there's no need to log an exception here.

andrewor14 · 2015-06-24T18:36:28Z

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

@@ -90,17 +90,25 @@ class SparkEnv (
  private var driverTmpDirToDelete: Option[String] = None

  private[spark] def stop() {
+
+    if(isStopped) return


might be better to rewrite this as

if (!isStopped) { // stop everything }

Heh, I would have gone with just

if (isStopped) { return }

to avoid the extra running indent but leave it.

andrewor14 · 2015-06-24T18:36:53Z

ok to test

SparkQA · 2015-06-24T18:40:28Z

Test build #35709 has finished for PR 6973 at commit b566b66.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

squito · 2015-06-24T18:53:06Z

Hi @rekhajoshm

The PR look reasonable (with the suggested changes from andrew). Could you also (a) change the title / description to be a more descriptive, eg. "Allow sc.stop() to be called multiple times" and also (b) add a test case to SparkContextSuite to prevent future regression?

thanks for working on this!

SparkQA · 2015-06-25T02:39:34Z

Test build #35745 has finished for PR 6973 at commit 380c5b0.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

rekhajoshm · 2015-06-25T03:41:42Z

Thanks for your quick inputs and review comments @sarutak @srowen @andrewor14 @squito .

@srowen – Based on my quick look yesterday, sc.stop() calls sparkEnv stop() which tries to stop the already stopped and as it is not idempotent, causes a spiral effect of uncaught_exception of 50 exit code. https://issues.apache.org/jira/browse/SPARK-2645

@squito Did as suggested.Please review.Thanks

SparkQA · 2015-06-25T05:18:19Z

Test build #35748 has finished for PR 6973 at commit 58dba70.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2015-06-25T10:17:19Z

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

+        outputCommitCoordinator.stop()
+        rpcEnv.shutdown()
+      } catch {
+        case NonFatal(e) =>


What's the value in adding this?

I think sean's point is that by catching the error, its no longer getting passed further up the stack. If something unexpected goes wrong here, we most probably don't want to just log it, its likely to get overlooked and maybe indicates something seriously wrong.

Yes, but further, I don't know if it helps to log it here either. I'd remove this try block.

I agree. Looks like the only thing this does is log the exception, but if we propagate it upwards it will be observed by the upper layers anyway. @rekhajoshm can you remove this try block?

srowen · 2015-06-25T10:18:26Z

Jenkins retest this please

SparkQA · 2015-06-25T12:54:41Z

Test build #35772 has finished for PR 6973 at commit 58dba70.

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

squito · 2015-06-25T15:07:25Z

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

+        // assert(!e.getMessage.contains("Server is already stopped"))
+        threwNoOrOnlyExceptedException = false
+      case NonFatal(e) =>
+        threwNoOrOnlyExceptedException = true


what exception are you expecting here? I don't see any exception when running this test locally.

If you aren't expecting any exception, then you could just have the body of the try block in the test (any exception will fail the test).

If there is some "OK" exception, we should probably check more directly for that exception, as this will allow almost any exception to get thrown.

rekhajoshm · 2015-06-25T19:03:26Z

Thanks @srowen @squito for your review comments.
@srowen - log it for any kind of debugging, but can remove if not preferred
@squito - updated as suggested.

squito · 2015-06-25T19:30:26Z

hmm, now I am a little confused -- that test actually passes even without your changes. Sorry this may have been my fault for suggesting a test in SparkContextSuite -- I guess SparkContext itself already handles multiple calls to stop().

I'm not convinced this is still needed, but I suppose it doesn't hurt. In any case, I'd leave the test you have, and also add a case where you just stop the env multiple times.

vanzin · 2015-06-25T19:34:45Z

None of the changes here are in SparkContext; the PR title is misleading, since you're changing SparkEnv. What code path would cause SparkEnv.stop() to be called multiple times?

SparkContext.stop() is already idempotent:

if (!stopped.compareAndSet(false, true)) {
  logInfo("SparkContext already stopped.")
  return
}

SparkQA · 2015-06-25T21:33:21Z

Test build #35806 has finished for PR 6973 at commit 9193a0c.

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

rekhajoshm · 2015-06-26T05:18:51Z

thanks @vanzin for your comments.Updated the title.Other than SparkContext, SparkEnv stop can be invoked from Scheduler, or if Driver commanded a shutdown, Executor stop would invoke SparkEnv stop, hence making SparkEnv idempotent as well.

On SPARK-2645, org.apache.spark.ServerStateException: Server is already stopped, from my understanding unhandled exception surfaces as uncaught_exception (exit code: 50)
Issue may be erratic, even be due to somewhere a async code state not updated correctly, shutDown hook, or (wild) scala compiler optimization within steps, or only in machine/usage specific variants :-)
From my very quick analysis, the one apparent culprit was stop() not being idempotent on SparkEnv., the fix was to prevent stopped env to be stopped again.

@squito @srowen good point; in enthusiasm to avoid unhandled exception especially when I am not expecting any or only in erratic maneuvers, but completely agree with you.updated.

Thanks again @vanzin @squito @srowen !

SparkQA · 2015-06-26T07:28:53Z

Test build #35821 has finished for PR 6973 at commit a5a7d7f.

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

srowen · 2015-06-26T08:02:09Z

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

+      // call stop second time
+      sc.stop()
+    } catch {
+      case e: Exception =>


This try should be removed since it doesn't really add anything, except hiding the exception. Let it fly from the method to fail the test.

rekhajoshm · 2015-06-26T18:30:23Z

thanks @srowen for your inputs.I had missed capturing exception on fail.updated(12f66b5).try/catch/fail(e) seems to be accepted practice when failing on exception.Used org.scalatest.Matchers "noException should be thrownBy" if that is preferred (1aff39c).Or please let me know if you have another preference.

import org.scalatest.Matchers._
noException should be thrownBy {
    sc = new SparkContext(new SparkConf().setAppName("test").setMaster("local"))
    val cnt = sc.parallelize(1 to 4).count()
    sc.cancelAllJobs()
    sc.stop()
    // call stop second time
    sc.stop()
  }

SparkQA · 2015-06-26T18:31:03Z

Test build #35874 has finished for PR 6973 at commit 1aff39c.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-06-26T20:40:43Z

Test build #35873 has finished for PR 6973 at commit 12f66b5.

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

SparkQA · 2015-06-26T21:11:20Z

Test build #35875 has finished for PR 6973 at commit c97839a.

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

srowen · 2015-06-29T08:57:12Z

@rekhajoshm I think this is still in your court. The extra try blocks should be removed IMHO.

SparkQA · 2015-06-29T22:20:23Z

Test build #36022 has finished for PR 6973 at commit 2ce5760.

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

rekhajoshm · 2015-06-29T23:20:02Z

@srowen done.can you please review/approve it?thanks.

andrewor14 · 2015-06-30T00:31:25Z

@rekhajoshm LGTM other than a minor comment @srowen had.

rekhajoshm · 2015-06-30T02:16:14Z

thanks @andrewor14 for your quick input.the last commit 2ce5760 was updated for the comment from @srowen kindly check.thanks

rekhajoshm · 2015-06-30T03:24:52Z

had missed that inline comment. updated @andrewor14 @srowen thanks

SparkQA · 2015-06-30T05:59:13Z

Test build #36077 has finished for PR 6973 at commit 446b0a4.

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

srowen · 2015-06-30T06:28:02Z

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

@@ -45,6 +45,8 @@ import org.apache.spark.storage._
 import org.apache.spark.unsafe.memory.{ExecutorMemoryManager, MemoryAllocator}
 import org.apache.spark.util.{RpcUtils, Utils}

+import scala.util.control.NonFatal


Not needed now?

rekhajoshm · 2015-06-30T17:51:57Z

Thanks for your inputs @srowen @andrewor14. done.also removed unused JavaConversions._ please review, thanks.

SparkQA · 2015-06-30T20:44:41Z

Test build #36169 has finished for PR 6973 at commit 277043e.

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

andrewor14 · 2015-06-30T20:59:45Z

Merging into master. Thanks for the fix @rekhajoshm

rekhajoshm · 2015-06-30T21:37:49Z

thank you @andrewor14 @srowen for the reviews and merge.

rekhajoshm added 4 commits May 5, 2015 16:10

Merge pull request #1 from apache/master

e3677c9

Pulling functionality from apache spark

Merge pull request #2 from apache/master

106fd8e

pull latest from apache spark

Merge pull request #3 from apache/master

0be142d

Pulling functionality from apache spark

SPARK-2645: Fix for SparkContext stop behavior

b566b66

andrewor14 reviewed Jun 24, 2015
View reviewed changes

SPARK-2645: Fix for SparkContext stop behavior

380c5b0

SPARK-2645: Fix for SparkContext stop behavior

58dba70

rekhajoshm changed the title ~~[SPARK-2645] [Core] Fix for SparkContext stop behavior~~ [SPARK-2645] [Core] Allow sc.stop() to be called multiple times without side effects. Jun 25, 2015

srowen reviewed Jun 25, 2015
View reviewed changes

squito reviewed Jun 25, 2015
View reviewed changes

Fix for SparkContext stop behavior

9193a0c

rekhajoshm added 2 commits June 25, 2015 22:01

Fix for SparkContext stop behavior

a5a7d7f

Fix for SparkContext stop behavior

72bb484

rekhajoshm changed the title ~~[SPARK-2645] [Core] Allow sc.stop() to be called multiple times without side effects.~~ [SPARK-2645] [Core] Allow SparkEnv.stop() to be called multiple times without side effects. Jun 26, 2015

srowen reviewed Jun 26, 2015
View reviewed changes

rekhajoshm added 2 commits June 26, 2015 11:11

Fix for SparkContext stop behavior

12f66b5

Fix for SparkContext stop behavior

1aff39c

Fix for SparkContext stop behavior

c97839a

Fix for SparkContext stop behavior

2ce5760

Fix for SparkContext stop behavior

446b0a4

srowen reviewed Jun 30, 2015
View reviewed changes

Fix for SparkContext stop behavior

277043e

asfgit closed this in 7dda084 Jun 30, 2015

[SPARK-2645] [Core] Allow SparkEnv.stop() to be called multiple times without side effects. #6973

[SPARK-2645] [Core] Allow SparkEnv.stop() to be called multiple times without side effects. #6973

Conversation

rekhajoshm commented Jun 24, 2015

sarutak commented Jun 24, 2015

srowen commented Jun 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewor14 commented Jun 24, 2015

SparkQA commented Jun 24, 2015

squito commented Jun 24, 2015

SparkQA commented Jun 25, 2015

rekhajoshm commented Jun 25, 2015

SparkQA commented Jun 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srowen commented Jun 25, 2015

SparkQA commented Jun 25, 2015

Choose a reason for hiding this comment

rekhajoshm commented Jun 25, 2015

squito commented Jun 25, 2015

vanzin commented Jun 25, 2015

SparkQA commented Jun 25, 2015

rekhajoshm commented Jun 26, 2015

SparkQA commented Jun 26, 2015

Choose a reason for hiding this comment

rekhajoshm commented Jun 26, 2015

SparkQA commented Jun 26, 2015

SparkQA commented Jun 26, 2015

SparkQA commented Jun 26, 2015

srowen commented Jun 29, 2015

SparkQA commented Jun 29, 2015

rekhajoshm commented Jun 29, 2015

andrewor14 commented Jun 30, 2015

rekhajoshm commented Jun 30, 2015

rekhajoshm commented Jun 30, 2015

SparkQA commented Jun 30, 2015

Choose a reason for hiding this comment

rekhajoshm commented Jun 30, 2015

SparkQA commented Jun 30, 2015

andrewor14 commented Jun 30, 2015

rekhajoshm commented Jun 30, 2015