ulysses-you commented on code in PR #57329:
URL: https://github.com/apache/spark/pull/57329#discussion_r3627550829
##########
core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala:
##########
@@ -1247,6 +1318,11 @@ private[spark] class TaskSetManager(
// that the task is not running, and it is NetworkFailure rather
than TaskFailure.
case _ => !info.launching
}
+ // Grow the CPUs of the retry when a running task's executor died of a
JVM heap OOM. See
+ // the SparkOutOfMemoryError branch in handleFailedTask for the
non-fatal counterpart.
+ if (oomRetryCpusIncrement > 0 && !isBarrier && isOomExit &&
exitCausedByApp) {
Review Comment:
Fixed in 136abaa041a. `isOom` now walks a bounded cause chain instead of
inspecting only the top-level exception, reusing
`spark.executor.killOnFatalError.depth` as the bound (the same depth
`Executor.isFatalError` uses to find a fatal error in a wrapped exception),
with a top-level class-name fallback when the throwable was not preserved:
```scala
private def isOom(ef: ExceptionFailure, depthToCheck: Int): Boolean = {
ef.exception match {
case Some(t) => causedByOom(t, depthToCheck)
case None =>
ef.className == classOf[OutOfMemoryError].getName ||
ef.className == classOf[SparkOutOfMemoryError].getName
}
}
@scala.annotation.tailrec
private def causedByOom(t: Throwable, depthToCheck: Int): Boolean = {
if (depthToCheck <= 0) false
else t match {
case _: OutOfMemoryError => true // SparkOutOfMemoryError is a subclass
case e if e.getCause != null => causedByOom(e.getCause, depthToCheck - 1)
case _ => false
}
}
```
This recognizes both cases you raised: a wrapped `SparkOutOfMemoryError`
(non-fatal, no executor-loss fallback) now grows the retry, and a wrapped JVM
`OutOfMemoryError` sent as FAILED before the executor exits is recognized on
the FAILED path so the later OOM exit being skipped by the `info.running` guard
no longer loses the classification. Added two tests: `wrapped
SparkOutOfMemoryError increments numOomRetries` and `wrapped JVM
OutOfMemoryError FAILED before executor loss increments once` (the latter also
asserts the subsequent executor loss does not double-count).
One accepted limitation: if the throwable is not preserved (`preserveCause =
false`) the fallback only inspects the top-level class name, so a wrapped OOM
whose throwable was dropped is not recognized. Carrying an explicit OOM flag
from the executor would close that gap but requires a wire/protocol change;
happy to pursue that separately if you think it is worth it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
