[SPARK-21146] [CORE] Master/Worker should handle and shutdown when any thread gets UncaughtException #18357

devaraj-kavali · 2017-06-20T01:04:19Z

What changes were proposed in this pull request?

Adding the default UncaughtExceptionHandler to the Worker.

How was this patch tested?

I verified it manually, when any of the worker thread gets uncaught exceptions then the default UncaughtExceptionHandler will handle those exceptions.

gets UncaughtException

jiangxb1987 · 2017-06-20T09:36:52Z

Could you elaborate on when we need this improvement? In fact I don't see much benefit of having this at first glance.

devaraj-kavali · 2017-06-20T15:37:19Z

@jiangxb1987 Thanks for looking at this.

Any one of the threads in Worker gets an exception which is unhandled then the thread gets terminated and process(Worker) keeps running, and the Worker doesn't perform the terminated thread functionality. One instance I mentioned in the JIRA description, where dispatcher-event thread got the exception and terminated, worker keeps running without performing the terminated thread functionality. I think we should handle this at the process level for all the threads, if any thread gets unhandled exception then the process(Worker) should exit instead of performing partial functionality.

This PR code change would set the default UncaughtExceptionHandler as SparkUncaughtExceptionHandler and for any unhandled exception in any thread of the process, SparkUncaughtExceptionHandler handles the exception and exit the process with the appropriate exit code.

jiangxb1987 · 2017-06-21T01:19:19Z

AFAIK SparkUncaughtExceptionHandler is designed to make Executors fail fast, because Executors are relatively lightweight, but I'm not sure we can afford to let Workers, even the Master fail fast too.
cc @cloud-fan for more comments.

cloud-fan · 2017-06-21T01:58:11Z

according to the JIRA, if the worker meets some unrecoverable issues and the state becomes DEAD, it seems make sense to terminate this worker as it's not functional. But can you double check that this change won't terminate a still-functional worker?

devaraj-kavali · 2017-06-21T05:54:32Z

But can you double check that this change won't terminate a still-functional worker?

The change doesn't impact/harm the fully functional Worker, SparkUncaughtExceptionHandler handles only when any thread gets terminated due to the exception thrown from it without handling.

zsxwing · 2017-06-21T18:45:52Z

ok to test

zsxwing · 2017-06-21T18:46:16Z

~~LGTM. I thought we already did it :(~~ I'm taking it back. I didn't realized SparkUncaughtExceptionHandler also kills the process for non-fatal exceptions. Then this seems dangerous. I'm not sure if we forget catch any non-fatal exceptions. Could you add a flag to enable/disable it?

SparkQA · 2017-06-21T21:32:34Z

Test build #78404 has finished for PR 18357 at commit c6e71f8.

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

devaraj-kavali · 2017-06-21T21:50:32Z

@zsxwing Thanks for looking into this, I see SparkUncaughtExceptionHandler exits the process for all the exceptions/errors.

Can we modify SparkUncaughtExceptionHandler in such a way that it kills the process for all Errors(always) and kills the process for Exceptions only when the flag is true, this flag can be enabled for Executors and disabled for other daemons(Master, Worker and HistoryServer)?

zsxwing · 2017-06-21T21:54:08Z

@devaraj-kavali SparkUncaughtExceptionHandler is a singleton so you cannot get the configuration from SparkConf.

devaraj-kavali · 2017-06-21T22:04:46Z

can we change SparkUncaughtExceptionHandler to class and create instance in each daemons main() with the constructor flag whether to kill the process for Exceptions or not? Something like below,

For Executor :

Thread.setDefaultUncaughtExceptionHandler(new SparkUncaughtExceptionHandler(true)) // Here true denotes to kill the process for Exceptions

For Other Daemons:

Thread.setDefaultUncaughtExceptionHandler(new SparkUncaughtExceptionHandler(false)) // false denotes not to kill the process for Exceptions

kill/not-to-kill the process for the unhandled exceptions with the default value as true, behaviour remains same for unhandled Error instances.

SparkQA · 2017-06-29T01:16:49Z

Test build #78838 has finished for PR 18357 at commit a2f30f0.

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

jiangxb1987 · 2017-06-29T13:53:37Z

retest this please.

SparkQA · 2017-06-29T16:42:04Z

Test build #78912 has finished for PR 18357 at commit a2f30f0.

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

zsxwing · 2017-06-30T00:08:27Z

core/src/main/scala/org/apache/spark/util/SparkUncaughtExceptionHandler.scala

@@ -26,27 +26,34 @@ import org.apache.spark.internal.Logging
 */
 private[spark] object SparkUncaughtExceptionHandler
  extends Thread.UncaughtExceptionHandler with Logging {
+  private[this] var exitOnException = true


Instead of using a static variable, I prefer to change SparkUncaughtExceptionHandler to a class.

zsxwing · 2017-06-30T00:09:23Z

core/src/main/scala/org/apache/spark/util/SparkUncaughtExceptionHandler.scala

      }
-    } catch {
-      case oom: OutOfMemoryError => Runtime.getRuntime.halt(SparkExitCode.OOM)


Could you keep these codes? It unlikely happens but since the codes are there, it's better to not change it.

@zsxwing, this code is still there but I moved the try&catch to the block where we invoke System.exit. Do you mean moving the whole code in uncaughtException() to try block and having the catch block?

+ } catch { + case oom: OutOfMemoryError => Runtime.getRuntime.halt(SparkExitCode.OOM) + case t: Throwable => Runtime.getRuntime.halt(SparkExitCode.UNCAUGHT_EXCEPTION_TWICE)

Do you mean moving the whole code in uncaughtException() to try block and having the catch block?

Yes.

@zsxwing Thanks for the clarification.

SparkQA · 2017-06-30T21:41:08Z

Test build #78997 has finished for PR 18357 at commit 56c2e4d.

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

zsxwing · 2017-07-06T06:06:49Z

core/src/main/scala/org/apache/spark/util/SparkUncaughtExceptionHandler.scala

+        logError("[Process in shutdown] " + errMsg, exception)
+      } else if (exception.isInstanceOf[Error] ||
+        (!exception.isInstanceOf[Error] && exitOnException)) {
+        logError(errMsg + ". Shutting down now..", exception)
        if (exception.isInstanceOf[OutOfMemoryError]) {
          System.exit(SparkExitCode.OOM)
        } else {
          System.exit(SparkExitCode.UNCAUGHT_EXCEPTION)


The current changes are too much. Could you rename exitOnException to exitOnUncaughtException and just change this line to

if (exitOnUncaughtException) { System.exit(SparkExitCode.UNCAUGHT_EXCEPTION) }

zsxwing · 2017-07-06T06:07:25Z

core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala

@@ -737,6 +737,7 @@ private[deploy] object Worker extends Logging {
  val ENDPOINT_NAME = "Worker"

  def main(argStrings: Array[String]) {
+    Thread.setDefaultUncaughtExceptionHandler(new SparkUncaughtExceptionHandler(false))


nit: false => exitOnException = false

zsxwing · 2017-07-06T06:08:05Z

core/src/main/scala/org/apache/spark/util/SparkUncaughtExceptionHandler.scala

 */
-private[spark] object SparkUncaughtExceptionHandler
+private[spark] class SparkUncaughtExceptionHandler(val exitOnException: Boolean = true)


nit: please also document exitOnException using

@param exitOnException ...

zsxwing · 2017-07-06T06:08:32Z

core/src/main/scala/org/apache/spark/deploy/master/Master.scala

@@ -1037,6 +1037,7 @@ private[deploy] object Master extends Logging {
  val ENDPOINT_NAME = "Master"

  def main(argStrings: Array[String]) {
+    Thread.setDefaultUncaughtExceptionHandler(new SparkUncaughtExceptionHandler(false))


nit: false => exitOnException = false

zsxwing · 2017-07-06T21:07:42Z

LGTM. Pending tests. Thanks!

SparkQA · 2017-07-06T23:54:30Z

Test build #79298 has finished for PR 18357 at commit 3d5106b.

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

devaraj-kavali · 2017-07-11T18:47:22Z

@zsxwing Tests have passed, can you check this? Thanks

zsxwing · 2017-07-11T18:53:52Z

@devaraj-kavali Sorray. I forgot this PR. I will trigger a new run as master has been updated a lot. Will set a reminder for me to merge this PR :)

zsxwing · 2017-07-11T18:53:56Z

retest this please

SparkQA · 2017-07-11T21:13:02Z

Test build #79537 has finished for PR 18357 at commit 3d5106b.

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

zsxwing · 2017-07-11T21:29:41Z

retest this please

SparkQA · 2017-07-12T00:30:11Z

Test build #79542 has finished for PR 18357 at commit 3d5106b.

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

zsxwing · 2017-07-12T07:14:38Z

Thanks! Merging to master.

[SPARK-21146] [CORE] Worker should handle and shutdown when any thread

c6e71f8

gets UncaughtException

jiangxb1987 mentioned this pull request Jun 20, 2017

[SPARK-21148] [CORE] Set SparkUncaughtExceptionHandler to the Master #18358

Closed

Updated SparkUncaughtExceptionHandler to take an argument whether to

a2f30f0

kill/not-to-kill the process for the unhandled exceptions with the default value as true, behaviour remains same for unhandled Error instances.

zsxwing reviewed Jun 30, 2017

View reviewed changes

devaraj-kavali changed the title ~~[SPARK-21146] [CORE] Worker should handle and shutdown when any thread gets UncaughtException~~ [SPARK-21146] [CORE] Master/Worker should handle and shutdown when any thread gets UncaughtException Jun 30, 2017

Fixed the review comments

56c2e4d

zsxwing requested changes Jul 6, 2017

View reviewed changes

Fixed the review comments

3d5106b

asfgit closed this in e16e8c7 Jul 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-21146] [CORE] Master/Worker should handle and shutdown when any thread gets UncaughtException #18357

[SPARK-21146] [CORE] Master/Worker should handle and shutdown when any thread gets UncaughtException #18357

devaraj-kavali commented Jun 20, 2017

jiangxb1987 commented Jun 20, 2017

devaraj-kavali commented Jun 20, 2017

jiangxb1987 commented Jun 21, 2017

cloud-fan commented Jun 21, 2017

devaraj-kavali commented Jun 21, 2017

zsxwing commented Jun 21, 2017

zsxwing commented Jun 21, 2017 •

edited

Loading

SparkQA commented Jun 21, 2017

devaraj-kavali commented Jun 21, 2017

zsxwing commented Jun 21, 2017

devaraj-kavali commented Jun 21, 2017

SparkQA commented Jun 29, 2017

jiangxb1987 commented Jun 29, 2017

SparkQA commented Jun 29, 2017

zsxwing Jun 30, 2017

zsxwing Jun 30, 2017

devaraj-kavali Jun 30, 2017

zsxwing Jun 30, 2017

devaraj-kavali Jun 30, 2017

SparkQA commented Jun 30, 2017

zsxwing Jul 6, 2017

zsxwing Jul 6, 2017

zsxwing Jul 6, 2017

zsxwing Jul 6, 2017

zsxwing commented Jul 6, 2017

SparkQA commented Jul 6, 2017

devaraj-kavali commented Jul 11, 2017

zsxwing commented Jul 11, 2017

zsxwing commented Jul 11, 2017

SparkQA commented Jul 11, 2017

zsxwing commented Jul 11, 2017

SparkQA commented Jul 12, 2017

zsxwing commented Jul 12, 2017

[SPARK-21146] [CORE] Master/Worker should handle and shutdown when any thread gets UncaughtException #18357

[SPARK-21146] [CORE] Master/Worker should handle and shutdown when any thread gets UncaughtException #18357

Conversation

devaraj-kavali commented Jun 20, 2017

What changes were proposed in this pull request?

How was this patch tested?

jiangxb1987 commented Jun 20, 2017

devaraj-kavali commented Jun 20, 2017

jiangxb1987 commented Jun 21, 2017

cloud-fan commented Jun 21, 2017

devaraj-kavali commented Jun 21, 2017

zsxwing commented Jun 21, 2017

zsxwing commented Jun 21, 2017 • edited Loading

SparkQA commented Jun 21, 2017

devaraj-kavali commented Jun 21, 2017

zsxwing commented Jun 21, 2017

devaraj-kavali commented Jun 21, 2017

For Executor :

For Other Daemons:

SparkQA commented Jun 29, 2017

jiangxb1987 commented Jun 29, 2017

SparkQA commented Jun 29, 2017

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

Choose a reason for hiding this comment

SparkQA commented Jun 30, 2017

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

zsxwing commented Jul 6, 2017

SparkQA commented Jul 6, 2017

devaraj-kavali commented Jul 11, 2017

zsxwing commented Jul 11, 2017

zsxwing commented Jul 11, 2017

SparkQA commented Jul 11, 2017

zsxwing commented Jul 11, 2017

SparkQA commented Jul 12, 2017

zsxwing commented Jul 12, 2017

zsxwing commented Jun 21, 2017 •

edited

Loading