spark-test commented on code in PR #60149:
URL: https://github.com/apache/spark/pull/60149#discussion_r4496203936
##########
core/src/main/scala/org/apache/spark/MapOutputTracker.scala:
##########
@@ -912,7 +930,9 @@ private[spark] class MapOutputTrackerMaster(
.getOrElse(Seq.empty[BlockManagerId]))
}
} catch {
- case NonFatal(e) => logError(log"${MDC(ERROR, e.getMessage)}", e)
+ case NonFatal(e) =>
Review Comment:
**Non-blocking (P2):** This reply only runs for throwables that `NonFatal`
matches. If handling the request throws a `VirtualMachineError` or
`LinkageError` instead, for example an `OutOfMemoryError` while serializing a
large set of map statuses or a codec class that fails to initialize,
`replyFailure` is skipped. The error then leaves `run()` past the outer catch,
which only handles `InterruptedException`. `receiveAndReply` has already
returned, so the RPC `Inbox` won't answer either. The executor's `askTracker`
still waits out `spark.rpc.askTimeout` and fails with an RPC timeout that hides
the driver-side error, which is the failure this PR fixes for non-fatal errors.
`Inbox.process` already answers any `Throwable` from `receiveAndReply` with
`sendFailure` and then rethrows it. Doing the same here would close the gap
while fatal errors keep propagating as they do today:
```scala
} catch {
case NonFatal(e) =>
logError(log"${MDC(ERROR, e.getMessage)}", e)
replyFailure(data, e)
case t: Throwable =>
logError(log"${MDC(ERROR, t.getMessage)}", t)
replyFailure(data, t)
throw t
}
```
A fatal error would still end that `MessageLoop`, as it does now. The change
only makes sure the waiting request is answered first.
**Verification:**
- **Regression:** A request whose handling throws a fatal error, such as a
LinkageError raised from the mocked context's reply, results in sendFailure
with that same error on the request's context. This fails before the change,
because the reply is skipped for errors that NonFatal excludes. The rethrown
error ends the MessageLoop that handled the request, so the test must not rely
on that loop serving a later request.
- **Behavior:** Non-fatal handler failures are still answered with
sendFailure and no reply, a reply that throws is still answered with
sendFailure of that exception, and a failure reply that itself throws still
leaves the loop serving the next request.
- **Inspection:** After the failure reply, the catch rethrows every
throwable that NonFatal excludes, so fatal errors and InterruptedException
still leave MessageLoop.run() as they do on the base branch instead of being
swallowed.
##########
core/src/test/scala/org/apache/spark/MapOutputTrackerSuite.scala:
##########
@@ -208,6 +208,90 @@ class MapOutputTrackerSuite extends SparkFunSuite with
LocalSparkContext {
rpcEnv.shutdown()
}
+ // SPARK-60950: tests post directly to the master's MessageLoop, bypassing
the endpoint, so
+ // that the exception is thrown on the dispatcher thread rather than inside
the RPC Inbox (which
Review Comment:
**Nit (P3):** This reason doesn't match how these requests are handled. For
the three queued request kinds,
`MapOutputTrackerMasterEndpoint.receiveAndReply` only logs and posts the
request. So the handler runs, and throws, on a `map-output-dispatcher` thread
whether a request is posted directly or goes through the endpoint. The `Inbox`
sends a failure only when `receiveAndReply` itself throws, which never happens
for these requests. As written, the parenthetical suggests that requests going
through the endpoint already get a failure reply, the opposite of the bug this
PR fixes. The existing `remote fetch below max RPC message size` test goes
through `receiveAndReply` with a mocked context, and its reply also comes from
a `MessageLoop` thread. Posting directly just lets these tests drive
`MessageLoop` with mocked call contexts, including ones whose `reply` or
`sendFailure` throws, without setting up an `RpcEnv` and endpoint. Consider
giving that as the reason, and saying that handl
er failures are thrown on the `MessageLoop` thread on both routes. The testing
section of the PR description repeats the same reasoning.
**Verification:**
- **Inspection:** The rewritten comment says that handler failures are
thrown on the map-output-dispatcher MessageLoop thread for both direct and
endpoint-routed requests. It gives the use of mocked call contexts, not the
throwing thread, as the reason for posting directly.
--
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]
