xmg333 commented on PR #18463: URL: https://github.com/apache/dolphinscheduler/pull/18463#issuecomment-5560816314
Thanks for the review. I have removed the legacy whole-file fallback from the download path entirely. ### 1. Avoid invoking the whole-file RPC when the chunk method is unavailable The whole-file RPC is no longer invoked from the download path at all — not only when the chunk method is unavailable: - `LogClientDelegate.writeLocalLegacy()` and `LogClientDelegate.getWholeLogBytes()` are deleted. - `LocalLogClient.getWholeLog()`, the byte[] `RemoteLogClient.getWholeLog()`, and the now-unused `LoggerService.getLogBytes(...)` are deleted with them, so there is no remaining code path in the API that can issue `getTaskInstanceWholeLogFileBytes`. The RPC interface and the worker/master-side `LogServiceImpl` implementation are kept unchanged, so that an old API server keeps working against a new worker/master — the reverse direction of the rolling upgrade. ### 2. Remote storage first; explicit error otherwise When the first chunk fails at the transport level (an old worker fails the RPC for the missing method — the client sees a `MethodInvocationException`, identical to the real "Cannot find the ServerMethodInvoker" response), the only fallback is `RemoteLogClient.streamWholeLog`, which streams the archive from remote storage in bounded chunks. If remote storage cannot serve the log either, the download fails with: > `Worker upgrade required for large log download: chunked log RPC is not available on worker <host> and remote log storage also failed` (the remote failure is attached as the cause). When the worker answers with a structured non-SUCCESS code — so it demonstrably implements the chunk RPC — the same path reports the worker's response and the remote failure without the upgrade guidance, so operators on a fully upgraded cluster don't chase the wrong remedy. The fallback runs exactly once per call, outside the chunk loop's try/catch, so it cannot re-enter (your earlier point about the double fallback). ### 3. Small-log compatibility over the legacy RPC is not preserved Agreed that preserving it would require a mechanism to establish a safe size before requesting the payload. We verified none exists on an old worker: the whole-file RPC is the unbounded read itself, the line-based RPC bounds lines but not bytes (a single multi-GB line defeats any line-based probe), and the RPC framework has no stat/size method. So during the rolling window, downloading from a not-yet-upgraded worker requires remote log archiving; otherwise the download fails with the explicit error above regardless of size. Log viewing is unaffected (line-bounded), and full download resumes once the worker is upgraded. If this behavior change should be noted in the release notes or upgrade docs, I can add that. ### 4. Rolling-upgrade regression test Added `RollingUpgradeLogStreamingIntegrationTest`: an embedded Netty RPC server whose `ILogService` is a JDK proxy simulating an old worker. The chunk RPC fails (the server converts the throw into a fail response — wire-equivalent to a real old worker), while the whole-file RPC actually works and returns the real 9 MB payload, counting every invocation with an `AtomicInteger`. Two scenarios run a 9 MB download over the real wire: - remote storage serves the archive → the download streams from remote; whole-file counter == 0; - remote storage also fails → the download fails with the explicit upgrade error; whole-file counter == 0. The counter is not vacuous: if a future change reintroduced the whole-file call, the stub would return a successful 9 MB response and both the counter and the content assertions would fail. Unit tests in `LogClientDelegateTest` cover every branch of the new flow: first-chunk ERROR → remote, RPC throw → remote, remote failure with/without upgrade guidance, SUCCESS + 0-byte empty log is terminal, mid-stream failures throw, `LOG_FILE_NOT_FOUND` → remote, rotation, node gone. -- 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]
