[SPARK-14357] [CORE] Properly handle the root cause being a commit denied exception #12228

jasonmoore2k · 2016-04-07T02:11:00Z

What changes were proposed in this pull request?

When deciding whether a CommitDeniedException caused a task to fail, consider the root cause of the Exception.

How was this patch tested?

Added a test suite for the component that extracts the root cause of the error.
Made a distribution after cherry-picking this commit to branch-1.6 and used to run our Spark application that would quite often fail due to the CommitDeniedException.

…nied exception

jasonmoore2k · 2016-04-07T02:13:24Z

Should the matching for FetchFailedException, TaskKilledException & InterruptedException get similar treatment?

andrewor14 · 2016-04-08T00:44:07Z

ok to test

andrewor14 · 2016-04-08T00:45:27Z

core/src/test/scala/org/apache/spark/util/CausedBySuite.scala

+      case CausedBy(e) => e
+    }
+
+    // Assert


can you remove all these comments and blank lines? They're not super necessary

Sure thing, thanks.

andrewor14 · 2016-04-08T00:46:12Z

Looks great. Representing it in unapply is quite elegant.

…gain)

SparkQA · 2016-04-08T02:42:08Z

Test build #55288 has finished for PR 12228 at commit 64a4499.

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

jasonmoore2k · 2016-04-08T02:53:29Z

Flaky test? The second test that is running has passed that one.

SparkQA · 2016-04-08T03:14:30Z

Test build #55293 has finished for PR 12228 at commit d5ebd55.

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

jasonmoore2k · 2016-04-08T03:20:49Z

Jenkins, retest this please.

jasonmoore2k · 2016-04-08T03:37:01Z

@andrewor14, think it's safe to just retest? I don't think the fatal error was because of these changes. I don't have the permissions to start a test run.

srowen · 2016-04-08T11:03:19Z

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

          execBackend.statusUpdate(taskId, TaskState.KILLED, ser.serialize(TaskKilled))

-        case cDE: CommitDeniedException =>
+        case CausedBy(cDE: CommitDeniedException) =>


It's tidy, but seems like overkill if this is the only place in the code that checks if "T, or its immediate cause, is a FooException". case t: Throwable if t.getCause.isInstanceOf[CommitDeniedException] => is sufficient to handle the additional case, at the cost of repeating the line of code.

Alternatively, if there are really a few instances of this pattern in the code that we can clean up with this pattern, then it seems worthwhile.

@srowen this is useful in other places too, so I don't think it's overkill. E.g. HiveExternalCatalog has some similar logic that could benefit from something like this in the future. What you suggested handles only 1 level of nesting and is less robust. I would prefer to leave this the way it is.

This also only handles 1 level, note. If it can be reused to improve other code that would be convincing.

it's recursive right? We call unapply inside unapply.

Oh I missed that, nice one. Yeah I agree this is a good one then, certainly if it can replace similar patterns elsewhere.

andrewor14 · 2016-04-08T17:52:35Z

retest this please

andrewor14 · 2016-04-08T18:01:04Z

retest this please

andrewor14 · 2016-04-08T18:05:52Z

Oh never mind, looks like Jenkins is broken at the moment: https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Compile/job/spark-master-compile-maven-hadoop-2.3/. I think @JoshRosen is fixing it ATM.

andrewor14 · 2016-04-08T19:16:16Z

Fixed. retest this please

SparkQA · 2016-04-08T21:09:28Z

Test build #55381 has finished for PR 12228 at commit d5ebd55.

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

andrewor14 · 2016-04-10T06:34:33Z

Merging into master, 1.6 and 1.5.

…ied exception ## What changes were proposed in this pull request? When deciding whether a CommitDeniedException caused a task to fail, consider the root cause of the Exception. ## How was this patch tested? Added a test suite for the component that extracts the root cause of the error. Made a distribution after cherry-picking this commit to branch-1.6 and used to run our Spark application that would quite often fail due to the CommitDeniedException. Author: Jason Moore <[email protected]> Closes #12228 from jasonmoore2k/SPARK-14357. (cherry picked from commit 22014e6) Signed-off-by: Andrew Or <[email protected]>

srowen · 2016-04-10T15:25:46Z

SGTM; I checked for other instances of this pattern and I don't see any, which is I suppose good news (didn't miss any) and bad (unfortunately don't get to simply anything else with this whole new class)

…ied exception ## What changes were proposed in this pull request? When deciding whether a CommitDeniedException caused a task to fail, consider the root cause of the Exception. ## How was this patch tested? Added a test suite for the component that extracts the root cause of the error. Made a distribution after cherry-picking this commit to branch-1.6 and used to run our Spark application that would quite often fail due to the CommitDeniedException. Author: Jason Moore <[email protected]> Closes apache#12228 from jasonmoore2k/SPARK-14357. (cherry picked from commit 22014e6) Signed-off-by: Andrew Or <[email protected]> (cherry picked from commit 7a02c44)

jasonmoore2k · 2016-04-26T06:24:47Z

@andrewor14 @srowen

I'm starting to think this change wasn't great to make. I'm now seeing speculated tasks sometimes getting into a cycle of running, being (legitimately) denied from committing their tasks, retrying, being denied, retrying, etc. And with the change I made in this PR, this retrying now has no limit. Which is really bad (jobs potentially running forever). Whereas previously they would eventually fail (still not great).

I think the ideal behavior would be to have the speculated task instead be considered a success if the commit is denied legitimately (because another task for the same partition has already completed), rather than considered a failure and retried without limit (to only fail again). Any thoughts?

srowen · 2016-04-26T06:52:00Z

You know more about the semantics than I do, but does this change make the behavior change in this way? it seems like just made something caused by a CDE be handled like a CDE. Are you suggesting just reverting this or some other change?

jasonmoore2k · 2016-04-26T07:24:37Z

Previously, code was in place to treat a CDE in a particular way, but it wasn't sufficient (it didn't consider that the CDE was actually nested inside a Spark Exception at the point it was being handled). This PR fixed that, but it seems to have highlighted a problem with the originally intended behavior (worst case can cause an infinite loop).

Reverting my change could return it to a state where speculation would often cause a job to be aborted (but at least finish), which is still not great.

Alternatively, I'm thinking we could put a task that receives a CDE from the driver, into a TaskState.FINISHED or some other state to indicated that the task shouldn't be resubmitted by the TaskScheduler. I'd probably need some opinions on whether there are other consequences for doing something like this.

It might be worth moving this discussion to a JIRA ticket. I'll fire one up.

jasonmoore2k · 2016-04-26T07:48:34Z

I've opened this ticket: https://issues.apache.org/jira/browse/SPARK-14915

[SPARK-14357] [CORE] Properly handle the root cause being a commit de…

64a4499

…nied exception

andrewor14 reviewed Apr 8, 2016
View reviewed changes

jasonmoore2k added 3 commits April 8, 2016 11:10

[SPARK-14357] [CORE] Removed some superfluous comments

d1fde47

[SPARK-14357] [CORE] Make usage comment better reflect style guide

afa7453

[SPARK-14357] [CORE] Make usage comment better reflect style guide (a…

d5ebd55

…gain)

srowen reviewed Apr 8, 2016
View reviewed changes

asfgit closed this in 22014e6 Apr 10, 2016

jasonmoore2k mentioned this pull request May 5, 2016

[SPARK-14915] [CORE] Don't re-queue a task if another attempt has already succeeded #12751

Closed

[SPARK-14357] [CORE] Properly handle the root cause being a commit denied exception #12228

[SPARK-14357] [CORE] Properly handle the root cause being a commit denied exception #12228

Uh oh!

Conversation

jasonmoore2k commented Apr 7, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

jasonmoore2k commented Apr 7, 2016

Uh oh!

andrewor14 commented Apr 8, 2016

Uh oh!

andrewor14 Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

jasonmoore2k Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Apr 8, 2016

Uh oh!

SparkQA commented Apr 8, 2016

Uh oh!

jasonmoore2k commented Apr 8, 2016

Uh oh!

SparkQA commented Apr 8, 2016

Uh oh!

jasonmoore2k commented Apr 8, 2016

Uh oh!

jasonmoore2k commented Apr 8, 2016

Uh oh!

srowen Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

andrewor14 Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

srowen Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

andrewor14 Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

srowen Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Apr 8, 2016

Uh oh!

andrewor14 commented Apr 8, 2016

Uh oh!

andrewor14 commented Apr 8, 2016

Uh oh!

andrewor14 commented Apr 8, 2016

Uh oh!

SparkQA commented Apr 8, 2016

Uh oh!

andrewor14 commented Apr 10, 2016

Uh oh!

srowen commented Apr 10, 2016

Uh oh!

jasonmoore2k commented Apr 26, 2016

Uh oh!

srowen commented Apr 26, 2016

Uh oh!

jasonmoore2k commented Apr 26, 2016

Uh oh!

jasonmoore2k commented Apr 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants