perf: RemoteTaskActionClient streaming deserialization. - #20005
Conversation
This patch switches RemoteTaskActionClient to use InputStreamResponseHandler rather than BytesFullResponseHandler, which enables deserialization to run without buffering the entire response. This is useful for actions with potentially large responses, such as RetrieveUsedSegmentsAction.
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 2 of 2 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
| response.get("result"), | ||
| taskAction.getReturnTypeReference() | ||
| ); | ||
| new InputStreamResponseHandler() |
There was a problem hiding this comment.
[P2] Streaming failures bypass ServiceClient retries
InputStreamResponseHandler completes the service future before the response body has finished. If the body fails mid-stream, deserialization receives an IOException after the future has already completed, so the configured RPC retry policy is bypassed; the previous BytesFullResponseHandler let ServiceClient retry these failures. Complete the response only after the stream is fully consumed, or preserve the retryable failure path.
There was a problem hiding this comment.
IMO this is OK, for two reasons.
-
Most failures are going to happen before the response body starts being sent. Generally once that starts, most failures we see would be timeouts or OOMs due to too much data being sent, which isn't generally going to be fixed by a retry anyway. There is precedent for not retrying these failures, especially in
CoordinatorClientImplandOverlordClientImpl. -
The alternative is the prior state (not releasing bytes to the JSON parser until they have been fully buffered). In theory it would be possible to incrementally parse the bytes inside an
HttpResponseHandlerthat returns an unfinished response, but I don't see a clean way to do this with Jackson. TheJsonParsermethods that drive parsing all expect to block while they fetch all their input, which would necessitate another thread, which I'd like to avoid.NonBlockingJsonParseris a thing that does exist in Jackson, but it doesn't seem to work withjackson-databind.
This patch switches RemoteTaskActionClient to use InputStreamResponseHandler rather than BytesFullResponseHandler, which enables deserialization to run without buffering the entire response. This is useful for actions with potentially large responses, such as RetrieveUsedSegmentsAction.