-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-13931] Stage can hang if an executor fails while speculated tasks are running #16855
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ | |
|
|
||
| package org.apache.spark.scheduler | ||
|
|
||
| import java.util.Random | ||
| import java.util.{Properties, Random} | ||
|
|
||
| import scala.collection.mutable | ||
| import scala.collection.mutable.ArrayBuffer | ||
|
|
@@ -28,6 +28,7 @@ import org.mockito.Mockito.{mock, never, spy, verify, when} | |
| import org.apache.spark._ | ||
| import org.apache.spark.internal.config | ||
| import org.apache.spark.internal.Logging | ||
| import org.apache.spark.serializer.SerializerInstance | ||
| import org.apache.spark.storage.BlockManagerId | ||
| import org.apache.spark.util.{AccumulatorV2, ManualClock} | ||
|
|
||
|
|
@@ -664,6 +665,63 @@ class TaskSetManagerSuite extends SparkFunSuite with LocalSparkContext with Logg | |
| assert(thrown2.getMessage().contains("bigger than spark.driver.maxResultSize")) | ||
| } | ||
|
|
||
| test("taskSetManager should not send Resubmitted tasks after being a zombie") { | ||
| // Regression test for SPARK-13931 | ||
| val conf = new SparkConf().set("spark.speculation", "true") | ||
| sc = new SparkContext("local", "test", conf) | ||
|
|
||
| val sched = new FakeTaskScheduler(sc, ("execA", "host1"), ("execB", "host2")) | ||
| sched.initialize(new FakeSchedulerBackend() { | ||
| override def killTask(taskId: Long, executorId: String, interruptThread: Boolean): Unit = {} | ||
| }) | ||
|
|
||
| // count for Resubmitted tasks | ||
| var resubmittedTasks = 0 | ||
| val dagScheduler = new FakeDAGScheduler(sc, sched) { | ||
|
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. Rather than defining your own DAGScheduler, can you use the existing FakeDAGSCheduler, and then use the FakeTaskScheduler to make sure that the task was recorded as ended for the correct reason? (i.e., not for the reason of being resubmitted)?
Author
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. @kayousterhout if I use the existing FakeDAGScheduler, I'll remove variable 'resubmittedTasks', then I can't make this test failed before my code modified.
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. It doesn't work to check that the TaskEndReason was success (and not Resubmitted), like is done here: https://github.com/GavinGavinNo1/spark/blob/24d8d795d26a5b1477cac01f2748c25fb9b74dc5/core/src/test/scala/org/apache/spark/scheduler/TaskSetManagerSuite.scala#L226 ?
Author
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. I still don't understand. I'm so confused about how to construct a failed test case before code modified, if I modify it below.
Author
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.
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. I see I played around with this a bit and the problem is that the TaskSetManager also sends an ExecutorLost task failure for the task that gets resubmitted, so that failure overrides the saved Resubmitted task end reason. It's fine to leave the existing test, but can you just add a comment that says something like "Keep track of the number of tasks that are resubmitted, so that the test can check that no tasks were resubmitted." |
||
| override def taskEnded(task: Task[_], reason: TaskEndReason, result: Any, | ||
| accumUpdates: Seq[AccumulatorV2[_, _]], taskInfo: TaskInfo): Unit = { | ||
|
||
| super.taskEnded(task, reason, result, accumUpdates, taskInfo) | ||
| reason match { | ||
| case Resubmitted => resubmittedTasks += 1 | ||
| case _ => | ||
| } | ||
| } | ||
| } | ||
| sched.setDAGScheduler(dagScheduler) | ||
|
|
||
| val singleTask = new ShuffleMapTask(0, 0, null, new Partition { | ||
| override def index: Int = 0 | ||
| }, Seq(TaskLocation("host1", "execA")), new Properties, null) | ||
| val taskSet = new TaskSet(Array(singleTask), 0, 0, 0, null) | ||
| val manager = new TaskSetManager(sched, taskSet, MAX_TASK_FAILURES) | ||
|
|
||
| // Offer host1, which should be accepted as a PROCESS_LOCAL location | ||
| // by the one task in the task set | ||
| val task1 = manager.resourceOffer("execA", "host1", TaskLocality.PROCESS_LOCAL).get | ||
|
|
||
| // Mark the task as available for speculation, and then offer another resource, | ||
| // which should be used to launch a speculative copy of the task. | ||
| manager.speculatableTasks += singleTask.partitionId | ||
| val task2 = manager.resourceOffer("execB", "host2", TaskLocality.ANY).get | ||
|
|
||
| assert(manager.runningTasks === 2) | ||
| assert(manager.isZombie === false) | ||
|
|
||
| val directTaskResult = new DirectTaskResult[String](null, Seq()) { | ||
|
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. here, can you add a comment with something like "Complete one copy of the task, which should result in the task set manager being marked as a zombie, because at least one copy of its only task has completed." |
||
| override def value(resultSer: SerializerInstance): String = "" | ||
| } | ||
| // Complete one copy of the task, which should result in the task set manager | ||
| // being marked as a zombie, because at least one copy of its only task has completed. | ||
| manager.handleSuccessfulTask(task1.taskId, directTaskResult) | ||
| assert(manager.isZombie === true) | ||
| assert(resubmittedTasks === 0) | ||
| assert(manager.runningTasks === 1) | ||
|
|
||
| manager.executorLost("execB", "host2", new SlaveLost()) | ||
| assert(manager.runningTasks === 0) | ||
| assert(resubmittedTasks === 0) | ||
| } | ||
|
|
||
| test("speculative and noPref task should be scheduled after node-local") { | ||
| sc = new SparkContext("local", "test") | ||
| sched = new FakeTaskScheduler( | ||
|
|
||
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 the description here "[SPARK-13931] taskSetManager ..." (and then eliminate the comment below)