[SPARK-14454] Better exception handling while marking tasks as failed by sameeragarwal · Pull Request #12234 · apache/spark

sameeragarwal · 2016-04-07T06:40:35Z

What changes were proposed in this pull request?

This patch adds support for better handling of exceptions inside catch blocks if the code within the block throws an exception. For instance here is the code in a catch block before this change in WriterContainer.scala:

logError("Aborting task.", cause)
// call failure callbacks first, so we could have a chance to cleanup the writer.TaskContext.get().asInstanceOf[TaskContextImpl].markTaskFailed(cause)
if (currentWriter !=null) {
currentWriter.close()
}
abortTask()
thrownewSparkException("Task failed while writing rows.", cause)

If markTaskFailed or currentWriter.close throws an exception, we currently lose the original cause. This PR fixes this problem by implementing a utility function Utils.tryWithSafeCatch that suppresses (Throwable.addSuppressed) the exception that are thrown within the catch block and rethrowing the original exception.

How was this patch tested?

No new functionality added

SparkQA · 2016-04-07T06:43:37Z

Test build #55196 has finished for PR 12234 at commit eebd2ef.

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

rxin · 2016-04-07T06:59:51Z

so the thing is this exception doesn't make it to the driver. it would be great if the error message that made it to the driver can contain the error for both, and the original exception's cause. then users know there is another exception that failed during close/callback, and they can go look up in the executor for the full stacktrace

rxin · 2016-04-07T07:06:16Z

nice - didn't know this existed

SparkQA · 2016-04-07T07:08:36Z

Test build #55197 has finished for PR 12234 at commit 7964b2d.

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

SparkQA · 2016-04-07T07:13:58Z

Test build #55198 has finished for PR 12234 at commit 94b37f2.

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

srowen · 2016-04-07T08:09:24Z

+ * fail as well. This would then suppress the original/likely more meaningful
+ * exception from the original `out.write` call.
+ */
+ def tryWithSafeCatchAndFailureCallbacks[T](block: => T)(catchBlock: => Unit): T = {


This seems redundant with the method above; they can be unified right?

yes, thanks. folded these changes in the method above

SparkQA · 2016-04-07T09:31:53Z

Test build #55199 has finished for PR 12234 at commit c9aaff0.

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

sameeragarwal · 2016-04-08T01:56:29Z

@davies are there other occurrences of this pattern?

sameeragarwal · 2016-04-08T05:40:22Z

test this please

SparkQA · 2016-04-08T06:02:48Z

Test build #55297 has finished for PR 12234 at commit e41cae8.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-08T07:52:10Z

Test build #55323 has finished for PR 12234 at commit e41cae8.

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

davies · 2016-04-09T00:23:12Z

LGTM,
Merging this into master, thanks!

rxin · 2016-04-09T00:56:43Z

@sameeragarwal can you create one for 1.6 backport?

…failed Backports #12234 to 1.6. Original description below: ## What changes were proposed in this pull request? This patch adds support for better handling of exceptions inside catch blocks if the code within the block throws an exception. For instance here is the code in a catch block before this change in `WriterContainer.scala`: ```scala logError("Aborting task.", cause) // call failure callbacks first, so we could have a chance to cleanup the writer. TaskContext.get().asInstanceOf[TaskContextImpl].markTaskFailed(cause) if (currentWriter != null) { currentWriter.close() } abortTask() throw new SparkException("Task failed while writing rows.", cause) ``` If `markTaskFailed` or `currentWriter.close` throws an exception, we currently lose the original cause. This PR fixes this problem by implementing a utility function `Utils.tryWithSafeCatch` that suppresses (`Throwable.addSuppressed`) the exception that are thrown within the catch block and rethrowing the original exception. ## How was this patch tested? No new functionality added Author: Sameer Agarwal <sameer@databricks.com> Closes#12272 from sameeragarwal/fix-exception-1.6.

…failed Backports apache#12234 to 1.6. Original description below: ## What changes were proposed in this pull request? This patch adds support for better handling of exceptions inside catch blocks if the code within the block throws an exception. For instance here is the code in a catch block before this change in `WriterContainer.scala`: ```scala logError("Aborting task.", cause) // call failure callbacks first, so we could have a chance to cleanup the writer. TaskContext.get().asInstanceOf[TaskContextImpl].markTaskFailed(cause) if (currentWriter != null) { currentWriter.close() } abortTask() throw new SparkException("Task failed while writing rows.", cause) ``` If `markTaskFailed` or `currentWriter.close` throws an exception, we currently lose the original cause. This PR fixes this problem by implementing a utility function `Utils.tryWithSafeCatch` that suppresses (`Throwable.addSuppressed`) the exception that are thrown within the catch block and rethrowing the original exception. ## How was this patch tested? No new functionality added Author: Sameer Agarwal <sameer@databricks.com> Closesapache#12272 from sameeragarwal/fix-exception-1.6. (cherry picked from commit c12db0d)

sameeragarwal changed the title ~~[SPARK-14454] Better exception handling while marking tasks as failed~~[WIP][SPARK-14454] Better exception handling while marking tasks as failedApr 7, 2016

rxin reviewed Apr 7, 2016
View reviewed changes

sameeragarwal force-pushed the fix-exception branch from eebd2ef to 7964b2dCompare April 7, 2016 07:03

rxin reviewed Apr 7, 2016
View reviewed changes

Comment threadcore/src/main/scala/org/apache/spark/scheduler/Task.scala Outdated
rxinApr 7, 2016
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice - didn't know this existed

sameeragarwal force-pushed the fix-exception branch from 7964b2d to 94b37f2Compare April 7, 2016 07:11

Better exception handling while marking tasks as failed
c9aaff0

sameeragarwal force-pushed the fix-exception branch from 94b37f2 to c9aaff0Compare April 7, 2016 07:26

srowen reviewed Apr 7, 2016
View reviewed changes

CR
e41cae8

sameeragarwal changed the title ~~[WIP][SPARK-14454] Better exception handling while marking tasks as failed~~[SPARK-14454] Better exception handling while marking tasks as failedApr 8, 2016

asfgit closed this in 813e96eApr 9, 2016

sameeragarwal mentioned this pull request Apr 9, 2016
[SPARK-14454][1.6] Better exception handling while marking tasks as failed #12272
Closed

Uh oh!

Conversation

sameeragarwal commented Apr 7, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 7, 2016

Uh oh!

rxinApr 7, 2016

Choose a reason for hiding this comment

Uh oh!

rxinApr 7, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 7, 2016

Uh oh!

SparkQA commented Apr 7, 2016

Uh oh!

srowenApr 7, 2016

Choose a reason for hiding this comment

Uh oh!

sameeragarwalApr 8, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 7, 2016

Uh oh!

sameeragarwal commented Apr 8, 2016

Uh oh!

sameeragarwal commented Apr 8, 2016

Uh oh!

SparkQA commented Apr 8, 2016

Uh oh!

SparkQA commented Apr 8, 2016

Uh oh!

davies commented Apr 9, 2016

Uh oh!

rxin commented Apr 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants