LuciferYang commented on code in PR #13150:
URL: https://github.com/apache/gluten/pull/13150#discussion_r4113831443
##########
gluten-core/src/main/scala/org/apache/spark/shuffle/ShuffleManagerRouter.scala:
##########
@@ -111,7 +113,12 @@ private object ShuffleManagerRouter {
def get(shuffleId: Int): ShuffleManager = {
val manager = cache.get(shuffleId)
- assert(manager != null, s"Shuffle manager not registered for shuffle id:
$shuffleId")
+ if (manager == null) {
+ // An assert is a no-op on JVMs launched without -ea (Spark's default
for
+ // executors), which would turn a cache miss into a bare NPE at the
+ // delegation call site instead of this diagnostic.
+ throw new GlutenException(s"Shuffle manager not registered for shuffle
id: $shuffleId")
Review Comment:
You're right on both counts, thanks — and it changes the conclusion, so I'm
closing this.
I conflated Scala's `@elidable` `assert` with Java's `-ea`-gated one.
There's no `-Xelide-below` in the build, so the assertion is active and already
throws `AssertionError` with the shuffle id; there was never a bare NPE to fix.
And that behavior is actually what we want here: a `cache.get` miss is an
invariant violation (only the block-resolver paths can even reach it —
`getReader`/`getWriter` pre-store via `ensureShuffleManagerRegistered`), and
because `AssertionError` is an `Error` it fails the task loudly instead of
being caught by `fetchLocalBlocks`' `case e: Exception` and laundered into a
`FetchFailed`/stage recompute. Switching to `GlutenException` would break
exactly that by making it catchable. The premise doesn't hold, so closing.
--
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]
