DanielLeens commented on PR #11564: URL: https://github.com/apache/seatunnel/pull/11564#issuecomment-6391360339
Thanks @SEZ9 for the thorough pass, and for confirming the head status. I agree with all five points and have nothing that contradicts them. My own review on 2026-10-23 for the same head `d71ee0a433fb` raised the same items, so I am +1'ing rather than duplicating them. Two small additions so the author has the complete list in one place: 1. **Rejection of `executionService.execute(...)` (Low-Medium).** `Offload.start()` does not route a `RejectedExecutionException` (or any exception thrown by `execute(...)` itself) through `sendFailureResponse`, so the worker would wait for the invocation timeout. Wrapping the `execute(...)` call in `try/catch` and calling `sendFailureResponse(e)` closes that gap, and it pairs naturally with the dedicated/bounded executor discussion in your item 1. 2. **Suggested priority.** Items 1 and 2 (executor ownership/backlog and the Javadoc stating the real `Offload` contract) are the ones I would treat as must-fix before approval. The rest can ride in the same push. On CI: the `Build` check on `d71ee0a433fb` currently shows `failure`. The failing fork jobs are `all-connectors-it-2`, `all-connectors-it-6`, `transform-v2-it-part-2`, `hbase-connector-it` and `mysql-cdc-connector-it` (JDK 11 and 17), none of which exercise `ReportMetricsOperation`, so they look unrelated to this diff. I have not triaged the individual logs, though, so once the code changes are pushed please re-run only the failed jobs on the new head and share the result. @nielifeng, no new commit exists yet, so everything above will be re-checked once you push. If you disagree with any item (for example if you think the shared `OFFLOADABLE_EXECUTOR` is acceptable), a short note on the thread explaining the reasoning is perfectly fine. Thanks again for the work on this. -- 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]
