venkata91 commented on code in PR #58437:
URL: https://github.com/apache/spark/pull/58437#discussion_r3961103444
##########
core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala:
##########
@@ -1228,8 +1228,20 @@ private[spark] class TaskSetManager(
// pipelined set and aborts the whole group. Note isZombie already skips a
fully-complete
// producer's set; this guard also covers a PARTIALLY-complete producer
losing an executor on
// decommission.
+
+ // OR, not AND: a shuffle reliably stored off-executor (globally, or just
this one via a remote
+ // shuffle service) keeps its map output when the executor dies. The
per-shuffle bit only ever
+ // adds reliability (defaults to false, set true solely by an opting-in
manager), so a false
+ // there means "no info", not "unreliable".
+ val reliablyStored =
sched.sc.shuffleDriverComponents.supportsReliableStorage() ||
Review Comment:
Thanks @tdcmeehan. Good point. Let me frame the two options so we can align
on one.
Core issue: reliability is per-shuffle (one shuffle on the remote service,
another falling back to local disk), so we now have two knobs, the app-global
`supportsReliableStorage()` and the per-shuffle
`ShuffleHandle.isReliablyStored`. Question is how they compose.
_Option A:_ OR (current PR). reliable = global || per-shuffle. Simple,
minimal change. Correct as long as a manager sets global = true only when no
shuffle can fall back. Celeborn does exactly this
([apache/celeborn#3834](https://github.com/apache/celeborn/pull/3834) ties it
to the NEVER policy).
Downside: Spark can't enforce that invariant, so any manager reporting
global = true while still allowing fallback would keep map output that's
actually gone. It relies on every manager keeping both knobs consistent.
_Option B_: per-shuffle authoritative (tri-state). Make the handle
Option[Boolean] (None = no info) and resolve as
`handle.reliablyStored.getOrElse(global)`. A per-shuffle answer always wins;
global is only the fallback when the handle says nothing. This is also
backwards compatible as well.
- Some(true) + any global -> reliable
- Some(false) + global true -> not reliable
- None + global true -> reliable (legacy)
- else / global false -> not reliable
Slightly bigger change (Option on the handle, resolved once at
registerShuffle), but Spark's semantics are self-consistent for any manager, no
manager can hit the unsafe Some(false) + global true case, and it degrades
cleanly for managers at any adoption stage. Generalizes past Celeborn to
Uniffle/Gluten.
I lean towards B, it's the cleaner contract for the SPI and doesn't depend
on managers keeping two knobs in sync. Either way apache/celeborn#3834 stays
the right Celeborn-side fix. WDYT?
--
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]
