[SPARK-3224] FetchFailed reduce stages should only show up once in failed stages (in UI) #2127

rxin · 2014-08-26T05:39:28Z

This is a HOTFIX for 1.1.

…e failed stages UI.

rxin · 2014-08-26T05:39:39Z

cc @kayousterhout & @pwendell can you take a look

SparkQA · 2014-08-26T05:46:01Z

QA tests have started for PR 2127 at commit 1dd3eb5.

This patch merges cleanly.

rxin · 2014-08-26T05:49:38Z

core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala

-          import env.actorSystem.dispatcher
-          env.actorSystem.scheduler.scheduleOnce(
-            RESUBMIT_TIMEOUT, eventProcessActor, ResubmitFailedStages)
+        // It is likely that we receive multiple FetchFailed for a single stage (because we have


see the comment here explaining the problem

SparkQA · 2014-08-26T06:38:33Z

QA tests have finished for PR 2127 at commit 1dd3eb5.

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

pwendell · 2014-08-26T17:27:06Z

Yeah - this makes sense. Just to be clear though, this doesn't change the existing logic except to surround it with an if-else, correct?

pwendell · 2014-08-26T17:27:13Z

LGTM

rxin · 2014-08-26T17:31:16Z

Jenkibns, retest this please.

rxin · 2014-08-26T17:31:22Z

Yea it doesn't.

SparkQA · 2014-08-26T17:34:34Z

QA tests have started for PR 2127 at commit 1dd3eb5.

This patch merges cleanly.

kayousterhout · 2014-08-26T18:29:27Z

core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala

+          val mapStage = shuffleToMapStage(shuffleId)
+          if (mapId != -1) {
+            mapStage.removeOutputLoc(mapId, bmAddress)
+            mapOutputTracker.unregisterMapOutput(shuffleId, mapId, bmAddress)


Don't we want these two lines, even if the stage has already been marked as failed? It seems like the new failure could be telling us about a different dead map output

SparkQA · 2014-08-26T18:29:32Z

QA tests have finished for PR 2127 at commit 1dd3eb5.

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

kayousterhout · 2014-08-26T18:31:10Z

Also can you write a unit test for this? I think it should be pretty easy -- you can just check for doulby-receiving StageCompleted events. Our lack of unit tests in the scheduler code has historically led to many bugs for new patches...

kayousterhout · 2014-08-26T20:10:15Z

Jenkins, retest this please

On Tue, Aug 26, 2014 at 10:31 AM, Reynold Xin [email protected]
wrote:

Jenkibns, retest this please.

—
Reply to this email directly or view it on GitHub
#2127 (comment).

kayousterhout · 2014-08-26T20:10:46Z

Oh oops these emails arrived in the wrong order -- I thought your request
for testing had not been satisfied. Sorry for the duplication!

On Tue, Aug 26, 2014 at 1:10 PM, Kay Ousterhout [email protected]
wrote:

Jenkins, retest this please

On Tue, Aug 26, 2014 at 10:31 AM, Reynold Xin [email protected]
wrote:

Jenkibns, retest this please.

—
Reply to this email directly or view it on GitHub
#2127 (comment).

rxin · 2014-08-26T22:21:49Z

Pushed a new version. I agree we should add test for it, but that shouldn't block 1.1.

SparkQA · 2014-08-26T22:26:03Z

QA tests have started for PR 2127 at commit 3d3d356.

This patch merges cleanly.

SparkQA · 2014-08-26T23:21:31Z

QA tests have finished for PR 2127 at commit 3d3d356.

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

kayousterhout · 2014-08-27T00:00:52Z

core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala

+            ") for resubmision due to a fetch failure")
+
+          logInfo("The failed fetch was from " + mapStage + " (" + mapStage.name +
+            "); marking it for resubmission")


Can you combine this with the above now-very-redundant log msg?

Added unit test for SPARK-3224

rxin · 2014-08-27T02:11:45Z

@kayousterhout I looked into the racing condition we discussed offline. I think even in that case the scheduler is resilient.

If you look at newOrUsedStage, it gets the map output locs from MapOutputTracker this way:

      val serLocs = mapOutputTracker.getSerializedMapOutputStatuses(shuffleDep.shuffleId)
      val locs = MapOutputTracker.deserializeMapStatuses(serLocs)
      for (i <- 0 until locs.size) {
        stage.outputLocs(i) = Option(locs(i)).toList   // locs(i) will be null if missing
      }

The Option there guards against null locations.

SparkQA · 2014-08-27T02:16:23Z

QA tests have started for PR 2127 at commit 49282b3.

This patch merges cleanly.

kayousterhout · 2014-08-27T02:39:27Z

That code looks like it's just setting the output locations for the map stage... what about the following case:

(1) map stage runs
(2) reduce stage starts
(3) reduce task fails because map output A is lost
(4) map stage is restarted , with a single task for output A
(5) scheduler gets another message that a second reduce task failed because output B was missing.
(6) map stage finishes, and new reduce stage is started
(7) when the reduce stage tries to get the output locations, won't it get an exception because there's no location for output B?

rxin · 2014-08-27T02:57:42Z

I think that's fine since that just fails the executor code, which will result in another retry?

SparkQA · 2014-08-27T03:11:01Z

QA tests have finished for PR 2127 at commit 49282b3.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- "$FWDIR"/bin/spark-submit --class $CLASS "$
- class ExternalSorter(object):
- "$FWDIR"/bin/spark-submit --class $CLASS "$
- protected class AttributeEquals(val a: Attribute)

kayousterhout · 2014-08-27T03:26:22Z

Ah cool you're right!

kayousterhout · 2014-08-27T04:48:17Z

core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala

+          markStageAsFinished(failedStage, Some("Fetch failure"))
+          runningStages -= failedStage
+          // TODO: Cancel running tasks in the stage
+          logInfo(s"Marking $failedStage (${failedStage.name}) for resubmision " +


Shouldn't this be below? Other than this tiny thing this LGTM

I'll tune up these log messages and merge this - thanks.

SparkQA · 2014-08-27T05:01:07Z

QA tests have started for PR 2127 at commit effb1ce.

This patch merges cleanly.

…iled stages (in UI) This is a HOTFIX for 1.1. Author: Reynold Xin <[email protected]> Author: Kay Ousterhout <[email protected]> Closes #2127 from rxin/SPARK-3224 and squashes the following commits: effb1ce [Reynold Xin] Move log message. 49282b3 [Reynold Xin] Kay's feedback. 3f01847 [Reynold Xin] Merge pull request #2 from kayousterhout/SPARK-3224 796d282 [Kay Ousterhout] Added unit test for SPARK-3224 3d3d356 [Reynold Xin] Remove map output loc even for repeated FetchFaileds. 1dd3eb5 [Reynold Xin] [SPARK-3224] FetchFailed reduce stages should only show up once in the failed stages UI. (cherry picked from commit bf71905) Signed-off-by: Patrick Wendell <[email protected]>

SparkQA · 2014-08-27T05:56:13Z

QA tests have finished for PR 2127 at commit effb1ce.

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

…iled stages (in UI) This is a HOTFIX for 1.1. Author: Reynold Xin <[email protected]> Author: Kay Ousterhout <[email protected]> Closes apache#2127 from rxin/SPARK-3224 and squashes the following commits: effb1ce [Reynold Xin] Move log message. 49282b3 [Reynold Xin] Kay's feedback. 3f01847 [Reynold Xin] Merge pull request apache#2 from kayousterhout/SPARK-3224 796d282 [Kay Ousterhout] Added unit test for SPARK-3224 3d3d356 [Reynold Xin] Remove map output loc even for repeated FetchFaileds. 1dd3eb5 [Reynold Xin] [SPARK-3224] FetchFailed reduce stages should only show up once in the failed stages UI.

[SPARK-3224] FetchFailed reduce stages should only show up once in th…

1dd3eb5

…e failed stages UI.

rxin reviewed Aug 26, 2014
View reviewed changes

kayousterhout reviewed Aug 26, 2014
View reviewed changes

Remove map output loc even for repeated FetchFaileds.

3d3d356

kayousterhout reviewed Aug 27, 2014
View reviewed changes

kayousterhout and others added 3 commits August 26, 2014 17:18

Added unit test for SPARK-3224

796d282

Merge pull request #2 from kayousterhout/SPARK-3224

3f01847

Added unit test for SPARK-3224

Kay's feedback.

49282b3

kayousterhout reviewed Aug 27, 2014
View reviewed changes

Move log message.

effb1ce

asfgit closed this in bf71905 Aug 27, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-3224] FetchFailed reduce stages should only show up once in failed stages (in UI) #2127

[SPARK-3224] FetchFailed reduce stages should only show up once in failed stages (in UI) #2127

rxin commented Aug 26, 2014

rxin commented Aug 26, 2014

SparkQA commented Aug 26, 2014

rxin Aug 26, 2014

SparkQA commented Aug 26, 2014

pwendell commented Aug 26, 2014

pwendell commented Aug 26, 2014

rxin commented Aug 26, 2014

rxin commented Aug 26, 2014

SparkQA commented Aug 26, 2014

kayousterhout Aug 26, 2014

SparkQA commented Aug 26, 2014

kayousterhout commented Aug 26, 2014

kayousterhout commented Aug 26, 2014

kayousterhout commented Aug 26, 2014

rxin commented Aug 26, 2014

SparkQA commented Aug 26, 2014

SparkQA commented Aug 26, 2014

kayousterhout Aug 27, 2014

rxin commented Aug 27, 2014

SparkQA commented Aug 27, 2014

kayousterhout commented Aug 27, 2014

rxin commented Aug 27, 2014

SparkQA commented Aug 27, 2014

kayousterhout commented Aug 27, 2014

kayousterhout Aug 27, 2014

pwendell Aug 27, 2014

SparkQA commented Aug 27, 2014

SparkQA commented Aug 27, 2014

[SPARK-3224] FetchFailed reduce stages should only show up once in failed stages (in UI) #2127

[SPARK-3224] FetchFailed reduce stages should only show up once in failed stages (in UI) #2127

Conversation

rxin commented Aug 26, 2014

rxin commented Aug 26, 2014

SparkQA commented Aug 26, 2014

rxin Aug 26, 2014

Choose a reason for hiding this comment

SparkQA commented Aug 26, 2014

pwendell commented Aug 26, 2014

pwendell commented Aug 26, 2014

rxin commented Aug 26, 2014

rxin commented Aug 26, 2014

SparkQA commented Aug 26, 2014

kayousterhout Aug 26, 2014

Choose a reason for hiding this comment

SparkQA commented Aug 26, 2014

kayousterhout commented Aug 26, 2014

kayousterhout commented Aug 26, 2014

kayousterhout commented Aug 26, 2014

rxin commented Aug 26, 2014

SparkQA commented Aug 26, 2014

SparkQA commented Aug 26, 2014

kayousterhout Aug 27, 2014

Choose a reason for hiding this comment

rxin commented Aug 27, 2014

SparkQA commented Aug 27, 2014

kayousterhout commented Aug 27, 2014

rxin commented Aug 27, 2014

SparkQA commented Aug 27, 2014

kayousterhout commented Aug 27, 2014

kayousterhout Aug 27, 2014

Choose a reason for hiding this comment

pwendell Aug 27, 2014

Choose a reason for hiding this comment

SparkQA commented Aug 27, 2014

SparkQA commented Aug 27, 2014