xmg333 commented on PR #18463: URL: https://github.com/apache/dolphinscheduler/pull/18463#issuecomment-5302848138
Thanks for the review. I spent quite a bit of time debugging the previous `OPAQUE_KEY` fix. In practice, there were more race conditions around the per-channel opaque set and its lifecycle than I initially expected, especially around channel failures and concurrent request completion. Rather than adding more bookkeeping to handle each case, I ended up replacing that approach entirely. The new approach is simpler: each `ResponseFuture` records the channel it was sent on, and `FUTURE_TABLE` is the only place tracking in-flight requests. When a channel fails or closes, we scan the table and fail the futures belonging to that channel. Futures on other channels are left alone. The lifecycle is now: > A future stays in `FUTURE_TABLE` until it completes, and completion always removes it. This also fixes a few cases that were easy to miss with the previous approach: * A deserialization/response-processing error can no longer leave a request stuck in the table. * Timeout, interrupt, and write-failure paths don't need separate cleanup logic. * There is no longer any channel-attribute initialization or swap handling. The requested regression test is included: `concurrentRequestsSharingChannel_decoderExceptionFailsAll` It sends two concurrent requests over the same real Netty channel, triggers a malformed frame, and verifies that both callers receive the decoder error promptly. I also added `ResponseFutureTest.failAllForChannel_*` for channel isolation and already-completed futures. ### Other fixes While testing this, I found a few highly related issues and fixed them as well: * `TransporterDecoder`: `maxFrameSize` now covers the entire message (header + body), using `long` arithmetic. The default value is shared by the client and server configs. * Log rotation during download no longer silently truncates. If `offset > fileLength`, the worker reports the `LOG_TRUNCATED`. If the file disappears during the read, the existing `FileNotFoundException` is propagated directly. * Empty and missing logs are now handled differently. A 0-byte log is a valid empty file, while a missing file fails explicitly. The HTTP response head is written lazily so an early failure can still return JSON. * Concurrent downloads of the same archive use striped per-path locks, and the read is bounded by the size observed at the start. If the file is replaced during the transfer, the download fails instead of returning potentially inconsistent data. * The download endpoint now has a request-scoped async timeout. This avoids the servlet's default 30s timeout truncating longer downloads without changing the global timeout. ### Verification There are now 80 tests across the 4 modules, including regression tests for the cases above. -- 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]
