Raviston6296 commented on PR #60149:
URL: https://github.com/apache/spark/pull/60149#issuecomment-6685879848
LGTM. I checked a few things while going through it:
- Moving `take()` out of the inner `try` doesn't change interrupt handling:
`NonFatal` never matched `InterruptedException`, so it already went to the
outer `catch` before this change.
- No double reply in the normal flow: `context.reply(...)` is the last
statement of each handler, and its argument (`serializedMapStatus` etc.) is
evaluated before it. So `sendFailure` only runs when nothing was replied, or
when `reply` itself threw. For a `LocalNettyRpcCallContext` whose promise was
already completed, `sendFailure` would throw `IllegalStateException`, and
`replyFailure` catches and logs it.
One question, not blocking: on the executor, `askTracker` sits outside the
`try` in `MapOutputTrackerWorker.getStatuses` that maps errors to
`MetadataFetchFailedException`. So the new fast failure still surfaces as a
plain task failure (`SparkException("Error communicating with
MapOutputTracker", ...)`), not a fetch failure. That's the same classification
as the old `RpcTimeoutException`, only without the 120s wait. For the
unregistered-shuffle race you describe, would it be better for the master to
answer with something the worker turns into a `MetadataFetchFailedException`,
so the stage is resubmitted instead of the task being retried? That would be a
follow-up, if anything.
--
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]
