yugan95 commented on code in PR #3801:
URL: https://github.com/apache/celeborn/pull/3801#discussion_r3781653256
##########
cpp/celeborn/network/MessageDispatcher.cpp:
##########
@@ -178,7 +209,21 @@ folly::Future<std::unique_ptr<Message>>
MessageDispatcher::operator()(
this->pipeline_->write(std::move(toSendMsg));
Review Comment:
You're right, and this is the exact mechanism the description cites as the
Java analogue. `wangle::AsyncSocketHandler::write` returns an
**already-failed** future when `!socket_->good()` (`"socket is closed in
write()"`, `AsyncSocketHandler.h`), and otherwise fails it later from
`AsyncTransport::WriteCallback::writeErr`. Neither path necessarily flips
`closed_` before the trailing check, so discarding the future left the
registered promise pending until the request timeout — the hang this PR is
about.
Both send paths now attach `.thenError`, which removes the registry entry
and fails it retriably: `failPendingRequest(requestId, ...)` for rpc/push and
`failPendingFetch(streamChunkSlice, ...)` for fetch. Both are no-ops when the
entry is already gone (fulfilled, cleaned up or interrupted).
The message is phrased `"Failed to send request {}, errorMsg: {}"` to match
Java's `StdChannelListener`, so `ShuffleClientImpl::getPushDataFailCause` ->
`connectFail()` classifies it as `PUSH_DATA_CONNECTION_EXCEPTION_PRIMARY`
instead of the non-critical default.
Covered by two new tests using a `MockHandler` whose `write()` returns a
failed future: `sendRpcRequestFailedWriteFailsRequestRetriably` and
`sendFetchChunkRequestFailedWriteFailsRetriably` — each asserts the dispatcher
is still available (only the write failed) and the future is ready with a
retriable `CelebornException`.
--
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]
