xmg333 commented on PR #18463:
URL:
https://github.com/apache/dolphinscheduler/pull/18463#issuecomment-5265528733
Thanks for the three points — all have been addressed.
### 1. "Keep every fallback path memory-bounded"
All three paths are now memory-bounded:
* **Chunk path:** Didn't change.
* **Legacy whole-file path:** `TransporterDecoder` now validates
`bodyLength` against `maxFrameSize` (64 MB by default, configurable through
`NettyServerConfig` / `NettyClientConfig`) and throws `TooLongFrameException`
**before** allocating `new byte[bodyLength]`.
I chose to reject oversized legacy downloads explicitly when this limit is
exceeded. `writeLocalLegacy` detects the condition through `isFrameTooLarge()`,
which walks the full cause chain, and throws `IOException("exceeds the maximum
legacy download size")`. This avoids both silent truncation and falling back to
remote storage, which could potentially serve stale data.
Logs under approximately 47 MB continue to work normally on old workers.
The rejection only affects oversized logs on workers that have not yet been
upgraded to the chunked RPC.
* **Remote path:** `RemoteLogClient.streamWholeLog` downloads the archive to
a local file using streaming I/O and then pipes it to the output in 8 KB
chunks. There is no whole-file `byte[]` allocation.
Additionally, `NettyClientHandler.exceptionCaught` now completes the pending
`ResponseFuture` immediately with the original cause, tracked through a channel
`AttributeKey`. This ensures that `TooLongFrameException` propagates to the
caller immediately instead of waiting for the RPC timeout.
Without this fix, the original exception cause could be lost and replaced by
a `RemoteTimeoutException` with a null cause, making the `isFrameTooLarge()`
check unreachable.
**Test coverage:**
* `testStreamWholeLogLegacyTooLargePropagatesExplicitly`
* `testStreamWholeLogRemoteFallbackIsChunked`
* `TransporterDecoderTest` — frame-size rejection
* `NettyClientHandlerTest` — exception propagation
---
### 2. Do not execute the whole-file fallback twice after an error
`streamWholeLog` now uses a `needFallback` flag that is set inside the
`try/catch`. The fallback call (`writeLocalLegacy`) has been moved outside both
blocks.
This ensures that:
* The fallback is executed at most once.
* Exceptions from the fallback propagate directly.
* The fallback exception cannot re-enter the original `catch` block and
trigger a second execution.
This is covered by:
`testStreamWholeLogRpcThrowsFallsBackToLegacyThenRemote`
The test explicitly verifies:
`verify(localLogClient, times(1)).getWholeLog(...)`
---
### 3. PR title and description
Updated to:
**Stream task log download in bounded chunks to prevent OOM**
--
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]
