-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-5945] Spark should not retry a stage infinitely on a FetchFailedException #5636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from 1 commit
Commits
Show all changes
50 commits
Select commit
Hold shift + click to select a range
40aefbe
[SPARK-5945] Added map to track reasons for stage failures and suppor…
f8744be
Fixed method scoping error
8fe31e0
Made StageFailure private to spark scheduler
e0f8b55
Made fail() method public
729b7ef
Added config option for stageFailure count and documentation
d5fa622
Moved failure tracking to Stage class. Added clear of failre count up…
0335b96
Removed stale documentation and fixed some erroneous spacing
2b91940
Added test case for stage abort after N failures
914b2cb
Nit
77555b9
Added test that also validates that the listenerBus sees the JobFaile…
1243b65
updated to fix last few items
9052e39
Removed extraneous assert
673fcb2
Fixed a couple minor issues. Removed unecessary asserT
e26ae6e
Fixed a couple minor issues. Removed unecessary asserT
75952ea
Fixed missing maxStageFailures
bc88aa1
Updated to add test cases for multiple task failures within a Stage. …
7ff8b21
Typo fix
560a381
Merge remote-tracking branch 'upstream/master' into SPARK-5945
76f226a
Resolved merge conflicts. Now simply count the nubmer stage failures …
fe647d0
Style
ddfe46c
restoring lost files
ee8d52e
Updated test suite to properly create task sets and force stage failures
4da3d5d
got rid of println
5e13342
Updated tests for stage failures.
34d69fa
Merge remote-tracking branch 'upstream/master' into SPARK-5945
e101ed7
Updated test for fetch failures. Added validation of successfully gen…
daad2e4
Added a test for multiple fetch failures inside a single stage
0c054d3
Added a test for multiple fetch failures inside a single stage
f23c31b
Added test to ensure that stage failure only triggers with successive…
9978575
Test updates. Some tests still failing, unsure why.
b66d74e
Added stage ID checks in most places and fixed naming for attempts to…
2e058ba
All tests passing. Still need to refactor multiple fetch failures per…
f79011b
Updated remaining test for sequential test failures to have three sta…
62532fa
Nit fixes
1c1cb72
Style
17e85de
Naming
cf94850
refactored tests to eliminate reused code
7c6f60f
Style
13af970
Nit fixes.
09929da
Style
eb15503
Style
01d6841
Nits
1dd4840
Fixed merge conflict
4da18a1
Style fix
5e4fe99
Merge remote-tracking branch 'upstream/master' into SPARK-5945
f928ff3
Updated PR description and minor nits
e22ce7c
Updated with feedback from PR
2bd4138
Style fix
1d44e0c
Test case updates and nit fixes
5bb1ae6
Added more comments to clarify tricky test case
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,6 +96,30 @@ class DAGScheduler( | |
| // Stages that must be resubmitted due to fetch failures | ||
| private[scheduler] val failedStages = new HashSet[Stage] | ||
|
|
||
| // The maximum number of times to retry a stage before aborting | ||
| val maxStageFailures = 5 | ||
|
|
||
| // To avoid cyclical stage failures (see SPARK-5945) we limit the number of times that a stage | ||
| // may be retried. However, it only makes sense to limit the number of times that a stage fails | ||
| // if it's failing for the same reason every time. Therefore, track why a stage fails as well as | ||
| // how many times it has failed. | ||
| case class StageFailure(failureReason : String) { | ||
| var count = 1 | ||
| def fail() = { count += 1 } | ||
| def shouldAbort(): Boolean = { count >= maxStageFailures } | ||
|
|
||
| override def equals(other: Any): Boolean = | ||
| other match { | ||
| case that: StageFailure => that.failureReason.equals(this.failureReason) | ||
| case _ => false | ||
| } | ||
|
|
||
| override def hashCode: Int = failureReason.hashCode() | ||
| } | ||
|
|
||
| // Map to track failure reasons for a given stage (indexed by stage ID) | ||
| private[scheduler] val stageFailureReasons = new HashMap[Stage, HashSet[StageFailure]] | ||
|
|
||
| private[scheduler] val activeJobs = new HashSet[ActiveJob] | ||
|
|
||
| /** | ||
|
|
@@ -460,6 +484,10 @@ class DAGScheduler( | |
| logDebug("Removing stage %d from failed set.".format(stageId)) | ||
| failedStages -= stage | ||
| } | ||
| if (stageFailureReasons.contains(stage)) { | ||
| logDebug("Removing stage %d from failure reasons set.".format(stageId)) | ||
| stageFailureReasons -= stage | ||
| } | ||
| } | ||
| // data structures based on StageId | ||
| stageIdToStage -= stageId | ||
|
|
@@ -940,6 +968,29 @@ class DAGScheduler( | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Check whether we should abort the failedStage due to multiple failures for the same reason. | ||
| * This method updates the running count of failures for a particular stage and returns | ||
| * true if the number of failures for any single reason exceeds the allowable number | ||
| * of failures. | ||
| * @return An Option that contains the failure reason that caused the abort | ||
| */ | ||
| def shouldAbortStage(failedStage: Stage, failureReason: String): Option[String] = { | ||
| if (!stageFailureReasons.contains(failedStage)) | ||
| stageFailureReasons.put(failedStage, new HashSet[StageFailure]()) | ||
|
|
||
| val failures = stageFailureReasons.get(failedStage).get | ||
| val failure = StageFailure(failureReason) | ||
| failures.find(s => s.equals(failure)) match { | ||
| case Some(f) => f.fail() | ||
| case None => failures.add(failure) | ||
| } | ||
| failures.find(_.shouldAbort()) match { | ||
| case Some(f) => Some(f.failureReason) | ||
| case None => None | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Responds to a task finishing. This is called inside the event loop so it assumes that it can | ||
| * modify the scheduler's internal state. Use taskEnded() to post a task end event from outside. | ||
|
|
@@ -1083,8 +1134,13 @@ class DAGScheduler( | |
| markStageAsFinished(failedStage, Some(failureMessage)) | ||
| } | ||
|
|
||
| val shouldAbort = shouldAbortStage(failedStage, failureMessage) | ||
| if (disallowStageRetryForTest) { | ||
| abortStage(failedStage, "Fetch failure will not retry stage due to testing config") | ||
| } else if (shouldAbort.isDefined) { | ||
| abortStage(failedStage, s"Fetch failure - aborting stage. Stage ${failedStage.name} " + | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you remove the first sentence of this and also change "Failure reason" to say "Most recent failure reason:" |
||
| s"has failed the maximum allowable number of times: ${maxStageFailures}. " + | ||
| s"Failure reason: ${shouldAbort.get}") | ||
| } else if (failedStages.isEmpty) { | ||
| // Don't schedule an event to resubmit failed stages if failed isn't empty, because | ||
| // in that case the event will already have been scheduled. | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make this a conf? there is already
spark.task.maxFailures, so how aboutspark.stage.maxFailures? Also it should get added to the docs