SiriusTL666 commented on PR #12974: URL: https://github.com/apache/seatunnel/pull/12974#issuecomment-6397893781
Thanks for the thorough review — it was genuinely useful, and two of the four points were real bugs in my change rather than nitpicks. I have pushed a follow-up commit (`8f8f389b`) addressing all four, rebased onto current `dev`. **Issue 2 (Medium) — `read_timeout_ms` was a no-op on the sink: you are right.** `SocketClient` only holds an `OutputStream`; it never reads. `SO_TIMEOUT` cannot affect writes, so the option and its docs were misleading. Fixed by: - moving `READ_TIMEOUT_MS` out of `SocketCommonOptions` into `SocketSourceOptions` (source-only), - removing it from `SocketSinkFactory.optionRule()`, - removing the dead `setSoTimeout` call from `SocketClient`, - removing the row from both sink docs. There is now a test asserting `read_timeout_ms` is **not** in the sink option rule, so this cannot regress silently. **Issue 4 (Medium) — the regression test could pass without the fix: you are right, and I verified it.** I originally used TEST-NET-1. On this machine the route actually times out (3s), but I then tried the saturated-backlog approach and it exposed the flaw you predicted: on Windows a full accept backlog returns `ConnectException` **immediately**, so `open()` failed fast even with `connect(address, 0)`. The test was green without the fix — exactly the failure mode you described. Replaced with a deterministic assertion instead of timing: ```java Mockito.verify(created).connect(Mockito.any(InetSocketAddress.class), Mockito.eq(4321)); ``` I confirmed it fails when reverted: ``` Argument(s) are different! Wanted: ... 4321 ``` Also added, as requested: negative/zero `connect_timeout_ms` validation for both source and sink, a sink test that `read_timeout_ms` is not offered, and a test that the socket is closed when `connect` fails. **Issue 1 (Low) — unconnected socket not closed: fixed.** Both `SocketSourceReader.open()` and `SocketClient.createConnection()` now connect via a local variable inside try/catch and close it on failure, suppressing the close exception onto the original. There is a test asserting `close()` is called when `connect` throws. **Issue 3 (Low) — default-change wording: addressed.** Rephrased the docs: `0` means no application-level timeout, leaving the OS default, rather than "wait forever". I have also called out the 10s default explicitly in the PR description. I did not touch `incompatible-changes.md` — happy to add an entry there if a maintainer considers this a release-notable default change, just say the word. **Verification** - `mvn -pl seatunnel-connectors-v2/connector-socket test` — **14/14 pass** (9 factory, 5 reader). - `spotless:check` clean. - Fork `Build` CI triggered on the new head; I will follow up with the link and result. One note on your comment that nothing was built locally — completely fair. For what it is worth, the two things your review caught (the no-op sink option and the non-discriminating regression test) are exactly the class of problem reading does catch and running does not, since both were green locally. -- 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]
