Skip to content

Conversation

@LuciferYang
Copy link
Contributor

@LuciferYang LuciferYang commented Mar 9, 2025

What changes were proposed in this pull request?

After SPARK-51184 (#49913), the variables failedExecutor and reason defined in the function TaskSchedulerImpl#statusUpdate are not reassigned after being initialized to None.

var failedExecutor: Option[String] = None
var reason: Option[ExecutorLossReason] = None
synchronized {
try {
Option(taskIdToTaskSetManager.get(tid)) match {
case Some(taskSet) =>
assert(state != TaskState.LOST)
if (TaskState.isFinished(state)) {
cleanupTaskState(tid)
taskSet.removeRunningTask(tid)
if (state == TaskState.FINISHED) {
taskResultGetter.enqueueSuccessfulTask(taskSet, tid, serializedData)
} else if (Set(TaskState.FAILED, TaskState.KILLED).contains(state)) {
taskResultGetter.enqueueFailedTask(taskSet, tid, state, serializedData)
}
}
if (state == TaskState.RUNNING) {
taskSet.taskInfos(tid).launchSucceeded()
}
case None =>
logError(log"Ignoring update with state ${MDC(LogKeys.TASK_STATE, state)} for " +
log"TID ${MDC(LogKeys.TASK_ID, tid)} because its task set is gone (this is " +
log"likely the result of receiving duplicate task finished status updates) or its " +
log"executor has been marked as failed.")
}
} catch {
case e: Exception => logError("Exception in statusUpdate", e)
}
}

Therefore, failedExecutor.isDefined will always be false, and the following if branch will never be entered:

// Update the DAGScheduler without holding a lock on this, since that can deadlock
if (failedExecutor.isDefined) {
assert(reason.isDefined)
dagScheduler.executorLost(failedExecutor.get, reason.get)
backend.reviveOffers()
}
.

So, the this pr cleans it up.

Why are the changes needed?

Code cleanup.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass GitHub Actions

Was this patch authored or co-authored using generative AI tooling?

No

@github-actions github-actions bot added the CORE label Mar 9, 2025
@LuciferYang LuciferYang changed the title [SPARK-51444][CORE] Remove unreachable code from TaskSchedulerImpl#statusUpdate [SPARK-51444][CORE] Remove the unreachable if branch from TaskSchedulerImpl#statusUpdate Mar 9, 2025
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Thank you, @LuciferYang and @amoghantarkar .
Merged to master for Apache Spark 4.1.0.

@LuciferYang
Copy link
Contributor Author

Thanks @dongjoon-hyun and @amoghantarkar

@LuciferYang LuciferYang deleted the ts-statusUpdate branch May 2, 2025 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants