[SPARK-23894][CORE][SQL] Defensively clear ActiveSession in Executors #21185

squito · 2018-04-27T20:11:06Z

Because SparkSession.getActiveSession uses an InheritableThreadLocal,
the ThreadPool in executors might end up inheriting the SparkSession
from a driver thread. This leads to test failures as executors should
never have an active SparkSession. So in local mode, defensively clear
the active session.

Because SparkSession.getActiveSession uses an InheritableThreadLocal, the ThreadPool in executors might end up inheriting the SparkSession from a driver thread. This leads to test failures as executors should never have an active SparkSession. So in local mode, defensively clear the active session.

SparkQA · 2018-04-28T00:28:08Z

Test build #89939 has finished for PR 21185 at commit 2a4944f.

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

vanzin · 2018-04-27T21:40:50Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

+   * Only in local mode, we have to prevent the driver from setting the active SparkSession
+   * in the executor threads.  See SPARK-23894.
+   */
+  lazy val clearActiveSparkSessionMethod = if (Utils.isLocalMaster(conf)) {


vanzin · 2018-04-27T21:41:25Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

@@ -299,6 +316,9 @@ private[spark] class Executor(
      Thread.currentThread.setContextClassLoader(replClassLoader)
      val ser = env.closureSerializer.newInstance()
      logInfo(s"Running $taskName (TID $taskId)")
+      // When running in local mode, we might end up with the active session from the driver set on
+      // this thread, though we never should, so we defensively clear it.  See SPARK-23894.
+      clearActiveSparkSessionMethod.foreach(_.invoke(null))


Can this be done in the thread pool's thread factory instead?

ThreadFactories only create threads, they don't start them, so you can't do it directly there, as I need this to run inside the thread.

I thought about trying to do something fancier, like having the thread pool also start the thread and submit one runnable to do this, or having threads get promoted to the correct thread pool after this ran, but I felt like it was too error-prone for minimal gain.

cloud-fan · 2018-04-28T03:43:04Z

I think SparkSession is driver only, how do we access it in executor?

squito · 2018-04-30T13:52:29Z

@cloud-fan

I think SparkSession is driver only, how do we access it in executor?

that's the whole problem. Its only meant to be available on the driver, but it ends up getting set on the executor when running in local mode. Because it uses an inheritable thread local, when the executor thread pool creates a new thread, the executor thread ends up inheriting the active session of the driver.

In cluster mode, when the executor is a totally separate JVM, there is no problem.

vanzin · 2018-04-30T19:30:19Z

Tests haven't triggered, weird. LGTM pending tests.

vanzin · 2018-04-30T19:30:27Z

retest this please

SparkQA · 2018-05-01T00:10:46Z

Test build #4160 has finished for PR 21185 at commit 49783c5.

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

gatorsmile · 2018-05-01T00:18:53Z

cc @ericl too

ericl · 2018-05-01T00:24:49Z

This makes sense to me. It would be slightly to clear it where where the session is getting leaked through threads, but if that's hard then this looks good.

squito · 2018-05-01T01:52:09Z

Jenkins, retest this please

SparkQA · 2018-05-01T02:00:18Z

Test build #4161 has started for PR 21185 at commit 49783c5.

gatorsmile · 2018-05-01T04:41:16Z

LGTM pending Jenkins

jiangxb1987

LGTM. This should resolve the flakiness of BucketedWriteWithoutHiveSupportSuite, though we shall still try to find the place where the Thread is actually leaked.

jiangxb1987 · 2018-05-01T15:06:31Z

retest this please

gatorsmile · 2018-05-01T15:42:01Z

ok to test

gatorsmile · 2018-05-01T15:42:11Z

test this please

squito · 2018-05-01T16:19:38Z

I don't understand the comments about "leaked threads". the executor thread pool is allowed to create threads whenever it wants. You can play around with this example if you like: https://gist.github.com/squito/8fc6533b1eeeb48559302c5898ae2c1d

The only "leak" here is potentially using an InheritableThreadLocal for the activeSession at all.
SparkSession.getActiveSession is a public api so we probably don't want to change the thread-inheritance behavior that it currently has. You could change SparkSession.getActiveSession to instead do the same thing itself, walking up the parent threads, but stopping if it hits an executor thread. But, seems more complex for no gain.

SparkQA · 2018-05-01T19:57:02Z

Test build #89978 has finished for PR 21185 at commit 49783c5.

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

jkbradley · 2018-05-01T20:32:37Z

There have been several of these R test failures. May be from flakiness with CRAN; testing locally now (since I didn't see any recent bad commits in R).

cloud-fan · 2018-05-02T07:23:37Z

that's the whole problem. Its only meant to be available on the driver, but it ends up getting set on the executor when running in local mode.

Yea I think this is the root cause. I'm making a PR to ban SQLConf.get at executor side, shall we do the same thing for SparkSession? And fixes all the places that mistakenly access SparkSession in executor.

squito · 2018-05-02T14:25:33Z

Yea I think this is the root cause. I'm making a PR to ban SQLConf.get at executor side, shall we do the same thing for SparkSession? And fixes all the places that mistakenly access SparkSession in executor.

ah that makes sense. All the failures I observed w/ the executor accessing the active session were actually via SQLConf.get. I don't know of any others.

I think it makes sense to actively ban SparkSession.getActiveSession the same way. I can include that in this PR, or make another one (or I guess you can put it into your PR), whatever you prefer. I don't think there will be any existing uses that need to be fixed, as I'm pretty sure if you did access it, you'd get the exception we've seen in tests related to the listener bus:

08:36:34.694 Executor task launch worker for task 436 ERROR Executor: Exception in task 0.0 in stage 402.0 (TID 436)
java.lang.IllegalStateException: LiveListenerBus is stopped.
        at org.apache.spark.scheduler.LiveListenerBus.addToQueue(LiveListenerBus.scala:97)
        at org.apache.spark.scheduler.LiveListenerBus.addToStatusQueue(LiveListenerBus.scala:80)
        at org.apache.spark.sql.internal.SharedState.<init>(SharedState.scala:93)
        at org.apache.spark.sql.SparkSession$$anonfun$sharedState$1.apply(SparkSession.scala:117)
...

SparkQA · 2018-05-02T18:54:24Z

Test build #4169 has finished for PR 21185 at commit 49783c5.

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

squito · 2018-05-02T19:08:08Z

@jkbradley still seeing flakiness in R tests, in other PRs too. I'm not even sure how to interpret the failure. is it safe to ignore those? I can't see how this would be effecting R tests ...

gatorsmile · 2018-05-02T19:30:05Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

+   * Only in local mode, we have to prevent the driver from setting the active SparkSession
+   * in the executor threads.  See SPARK-23894.
+   */
+  private lazy val clearActiveSparkSessionMethod = if (Utils.isLocalMaster(conf)) {


Do we still need to call this if we can resolve the root cause in #21190, and issue an error promptly like what you said in #21185 (comment)?

I've added this check in #21190

agreed, I closed this PR

squito · 2018-05-08T15:34:32Z

I'm closing this in favor of #21190

squito mentioned this pull request Apr 27, 2018

[SPARK-23962][SQL][TEST] Fix race in currentExecutionIds(). #21041

Closed

vanzin reviewed Apr 28, 2018

View reviewed changes

review feedback

49783c5

jiangxb1987 approved these changes May 1, 2018

View reviewed changes

gatorsmile reviewed May 2, 2018

View reviewed changes

squito closed this May 8, 2018

[SPARK-23894][CORE][SQL] Defensively clear ActiveSession in Executors #21185

[SPARK-23894][CORE][SQL] Defensively clear ActiveSession in Executors #21185

Uh oh!

Conversation

squito commented Apr 27, 2018

Uh oh!

SparkQA commented Apr 28, 2018

Uh oh!

vanzin Apr 27, 2018

Choose a reason for hiding this comment

Uh oh!

vanzin Apr 27, 2018

Choose a reason for hiding this comment

Uh oh!

squito Apr 30, 2018

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Apr 28, 2018

Uh oh!

squito commented Apr 30, 2018

Uh oh!

vanzin commented Apr 30, 2018

Uh oh!

vanzin commented Apr 30, 2018

Uh oh!

SparkQA commented May 1, 2018

Uh oh!

gatorsmile commented May 1, 2018

Uh oh!

ericl commented May 1, 2018

Uh oh!

squito commented May 1, 2018

Uh oh!

SparkQA commented May 1, 2018

Uh oh!

gatorsmile commented May 1, 2018

Uh oh!

jiangxb1987 left a comment

Choose a reason for hiding this comment

Uh oh!

jiangxb1987 commented May 1, 2018

Uh oh!

gatorsmile commented May 1, 2018

Uh oh!

gatorsmile commented May 1, 2018

Uh oh!

squito commented May 1, 2018

Uh oh!

SparkQA commented May 1, 2018

Uh oh!

jkbradley commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented May 2, 2018

Uh oh!

squito commented May 2, 2018

Uh oh!

SparkQA commented May 2, 2018

Uh oh!

squito commented May 2, 2018

Uh oh!

gatorsmile May 2, 2018

Choose a reason for hiding this comment

Uh oh!

cloud-fan May 7, 2018

Choose a reason for hiding this comment

Uh oh!

squito May 8, 2018

Choose a reason for hiding this comment

Uh oh!

squito commented May 8, 2018

Uh oh!

Uh oh!

jkbradley commented May 1, 2018 •

edited

Loading