yugan95 commented on code in PR #3801:
URL: https://github.com/apache/celeborn/pull/3801#discussion_r3781663235
##########
cpp/celeborn/network/MessageDispatcher.cpp:
##########
@@ -163,6 +186,14 @@ folly::Future<std::unique_ptr<Message>>
MessageDispatcher::operator()(
}
}
+ // Fast path: the connection is already closed. Fail with a retriable error
+ // rather than asserting, so the caller's retry/failover logic can recover.
+ if (closed_.load()) {
+ return folly::makeFuture<std::unique_ptr<Message>>(
+ makeConnectionClosedException(fmt::format(
Review Comment:
Confirmed — `sendRpcRequestSync` re-threw through `CELEBORN_FAIL` (hardcoded
`isRetriable=false`) and the push/fetch `thenError` continuations flattened the
cause into a plain `std::runtime_error`, so no caller could observe the flag.
`TransportClient` now preserves it:
- `sendRpcRequestSync` and `fetchChunkAsync`'s catch use
`failPreservingRetriable(...)`, which carries the cause's `isRetriable`.
- the push/fetch `thenError` continuations use `toCallbackException(...)`,
forwarding a retriable `CelebornRuntimeError` as-is.
- the synchronous push catch uses `wrapCallbackException(...)`, which keeps
the flag while wrapping the contextual message.
A non-retriable cause still produces a byte-identical
`std::runtime_error(message)`, so `getPushDataFailCause` string matching is
unchanged (there's a regression assertion for
`PUSH_DATA_FAIL_PARTITION_NOT_FOUND`). The callback consumers only read
`what()`, so forwarding the exception rather than a copy of its message is
behaviour-neutral for them.
Added API-level tests in `TransportClientTest.cpp` with a `MockDispatcher`
that returns a retriable failure:
`sendRpcRequestSyncPreservesRetriableFailure`,
`pushDataAsyncPreservesRetriableFailure`,
`fetchChunkAsyncPreservesRetriableFailure`.
--
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]
