-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-4014] Add TaskContext.attemptNumber and deprecate TaskContext.attemptId #3849
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 3 commits
fd515a5
1e7a933
9d8d4d1
b2dffa3
cbe4d76
8c387ce
0b10526
eee6a45
1d43aa6
a180b88
38574d4
5cfff05
89d03e0
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 |
|---|---|---|
|
|
@@ -77,10 +77,13 @@ private[spark] class MesosExecutorBackend | |
|
|
||
| override def launchTask(d: ExecutorDriver, taskInfo: TaskInfo) { | ||
| val taskId = taskInfo.getTaskId.getValue.toLong | ||
| val attemptIdPlusData = taskInfo.getData.asReadOnlyByteBuffer() | ||
|
Contributor
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. And here's the corresponding receive-side of the Mesos trickiness. |
||
| val attemptId = attemptIdPlusData.getLong // Updates the position by 8 bytes | ||
| val data = attemptIdPlusData.slice() // Subsequence starting at the current position | ||
| if (executor == null) { | ||
| logError("Received launchTask but executor was null") | ||
| } else { | ||
| executor.launchTask(this, taskId, taskInfo.getName, taskInfo.getData.asReadOnlyByteBuffer) | ||
| executor.launchTask(this, taskId = taskId, attemptId = attemptId, taskInfo.getName, data) | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -635,7 +635,7 @@ class DAGScheduler( | |
| val rdd = job.finalStage.rdd | ||
| val split = rdd.partitions(job.partitions(0)) | ||
| val taskContext = | ||
| new TaskContextImpl(job.finalStage.id, job.partitions(0), 0, true) | ||
| new TaskContextImpl(job.finalStage.id, job.partitions(0), 0, 0, true) | ||
|
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 we use named argument for the two zeros and the true?
Contributor
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. Good call; I didn't do this for the test code, but this line is in DAGScheduler so it should use named arguments. |
||
| TaskContextHelper.setTaskContext(taskContext) | ||
| try { | ||
| val result = job.func(taskContext, rdd.iterator(split, taskContext)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,8 +44,9 @@ import org.apache.spark.util.Utils | |
| */ | ||
| private[spark] abstract class Task[T](val stageId: Int, var partitionId: Int) extends Serializable { | ||
|
|
||
| final def run(attemptId: Long): T = { | ||
| context = new TaskContextImpl(stageId, partitionId, attemptId, runningLocally = false) | ||
| final def run(taskId: Long, attemptId: Long): T = { | ||
|
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. would be great to add javadoc here
Contributor
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. Are the descriptions in TaskContext okay? If so, I'll just copy those for the parameters. |
||
| context = new TaskContextImpl(stageId = stageId, partitionId = partitionId, taskId = taskId, | ||
| attemptId = attemptId, runningLocally = false) | ||
| TaskContextHelper.setTaskContext(context) | ||
| context.taskMetrics.hostname = Utils.localHostName() | ||
| taskThread = Thread.currentThread() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| package org.apache.spark.scheduler.cluster.mesos | ||
|
|
||
| import java.io.File | ||
| import java.nio.ByteBuffer | ||
| import java.util.{ArrayList => JArrayList, List => JList} | ||
| import java.util.Collections | ||
|
|
||
|
|
@@ -290,13 +291,22 @@ private[spark] class MesosSchedulerBackend( | |
| .setType(Value.Type.SCALAR) | ||
| .setScalar(Value.Scalar.newBuilder().setValue(scheduler.CPUS_PER_TASK).build()) | ||
| .build() | ||
| // Encode the attemptId as part of the data payload, since there's not a MesosTaskInfo field | ||
| // to hold it: | ||
| val data = { | ||
|
Contributor
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. Here's the Mesos trickiness that I alluded to in the PR description.
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 we type data explicitly? |
||
| val serializedTask = task.serializedTask | ||
| val dataBuffer = ByteBuffer.allocate(8 + serializedTask.limit()) | ||
| dataBuffer.putLong(task.attemptId) | ||
| dataBuffer.put(serializedTask) | ||
|
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 we introduce another wrapper here instead? I'm imagine we will be adding more fields to serialize to Mesos executors, and it's a lot easier to maintain a struct then position with types and offsets.
Contributor
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. That's a good idea; putting the serialization / deserialization code in the same wrapper class will make it much easier to verify that it's correct / test it separately. I'll push a new commit to do this. |
||
| ByteString.copyFrom(dataBuffer) | ||
| } | ||
| MesosTaskInfo.newBuilder() | ||
| .setTaskId(taskId) | ||
| .setSlaveId(SlaveID.newBuilder().setValue(slaveId).build()) | ||
| .setExecutor(createExecutorInfo(slaveId)) | ||
| .setName(task.name) | ||
| .addResources(cpuResource) | ||
| .setData(ByteString.copyFrom(task.serializedTask)) | ||
| .setData(data) | ||
| .build() | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,13 +45,13 @@ class TaskContextSuite extends FunSuite with BeforeAndAfter with LocalSparkConte | |
| val task = new ResultTask[String, String]( | ||
| 0, sc.broadcast(closureSerializer.serialize((rdd, func)).array), rdd.partitions(0), Seq(), 0) | ||
| intercept[RuntimeException] { | ||
| task.run(0) | ||
| task.run(0, 0) | ||
| } | ||
| assert(TaskContextSuite.completed === true) | ||
| } | ||
|
|
||
| test("all TaskCompletionListeners should be called even if some fail") { | ||
| val context = new TaskContextImpl(0, 0, 0) | ||
| val context = new TaskContextImpl(0, 0, 0, 0) | ||
| val listener = mock(classOf[TaskCompletionListener]) | ||
| context.addTaskCompletionListener(_ => throw new Exception("blah")) | ||
| context.addTaskCompletionListener(listener) | ||
|
|
@@ -63,6 +63,25 @@ class TaskContextSuite extends FunSuite with BeforeAndAfter with LocalSparkConte | |
|
|
||
| verify(listener, times(1)).onTaskCompletion(any()) | ||
| } | ||
|
|
||
| test("TaskContext.attemptId should return attempt number, not task id (SPARK-4014)") { | ||
| sc = new SparkContext("local-cluster[2,1,512]", "test") | ||
|
Contributor
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. Just realized that I can change the |
||
| // Check that attemptIds are 0 for all tasks' initial attempts | ||
| val attemptIds = sc.parallelize(Seq(1, 2), 2).mapPartitions { iter => | ||
| Seq(TaskContext.get().attemptId()).iterator | ||
| }.collect() | ||
| assert(attemptIds.toSet === Set(0)) | ||
|
|
||
| // Test a job with failed tasks | ||
| val attemptIdsWithFailedTask = sc.parallelize(Seq(1, 2), 2).mapPartitions { iter => | ||
| val attemptId = TaskContext.get().attemptId() | ||
| if (iter.next() == 1 && attemptId == 0) { | ||
| throw new Exception("First execution of task failed") | ||
| } | ||
| Seq(attemptId).iterator | ||
| }.collect() | ||
| assert(attemptIdsWithFailedTask.toSet === Set(0, 1)) | ||
| } | ||
| } | ||
|
|
||
| private object TaskContextSuite { | ||
|
|
||
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.
the toLong's are not necessary, are they?
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.
Good catch; this was a carryover from the old code.
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.
It turns out that
attemptIdwas a long but we only expected it to hold values that could fit in an int, hence this weird.toLongin a few places, plus a few% Int.MaxValuemodulus calls in various places; I've cleaned this up.