nzw921rx commented on PR #11214:
URL: https://github.com/apache/seatunnel/pull/11214#issuecomment-4843251206
> Thanks for the update. I re-reviewed the latest head from scratch against
the full current diff and re-traced the real Socket sink path again instead of
looking only at the new commit metadata.
>
> ## What this PR is trying to solve
> * User pain: invalid Socket sink configs such as `port <= 0` should fail
earlier instead of surfacing later in the runtime path.
> * Fix approach: move part of the validation into
`SocketSinkFactory.optionRule()` and add validator-based tests.
> * One-line summary: the declarative-validation direction is good, but the
current head still changes the historical `max_retries = 0` contract instead of
only moving validation earlier.
>
> ## Full runtime path I rechecked
> ```
> job config parsing
> -> SocketSinkFactory.optionRule()
> -> host required
> -> port must be > 0
> -> max_retries must be > 0 <-- current new gate
>
> sink creation
> -> createSink(...)
> -> new SocketSink(...)
> -> new SocketConfig(ReadonlyConfig)
> -> reads MAX_RETRIES
>
> write-time failure path
> -> SocketSinkWriter.write(...)
> -> SocketClient.write(...)
> -> old behavior:
> max_retries == 0 meant "do not retry; fail immediately"
> -> current behavior:
> zero is rejected before the sink is even created
> ```
>
> ## Key findings
> 1. The normal path definitely hits this change, because every Socket sink
job goes through `optionRule()` before writer creation.
> 2. `port > 0` is a reasonable declarative-validation improvement.
> 3. The compatibility blocker from my previous Daniel review is still
present on the latest head:
>
> * `SocketSinkFactory.java:41-44` still uses
`Conditions.greaterThan(MAX_RETRIES, 0)`
> * `SocketClient.java:77-89` no longer keeps the old `maxNumRetries ==
0` fail-fast branch
> * `SocketFactoryTest.java:82-93` still codifies `max_retries = 0` as
invalid
> 4. So the latest head still turns a historically valid config into a
submission-time validation failure. That is a user-visible contract change, not
just a validation cleanup.
>
> ## Findings
> ### Issue 1: the PR still removes the historical fail-fast contract for
`max_retries = 0`
> * Location:
>
> *
`seatunnel-connectors-v2/connector-socket/src/main/java/org/apache/seatunnel/connectors/seatunnel/socket/sink/SocketSinkFactory.java:41-44`
> *
`seatunnel-connectors-v2/connector-socket/src/main/java/org/apache/seatunnel/connectors/seatunnel/socket/sink/SocketClient.java:77-89`
> *
`seatunnel-connectors-v2/connector-socket/src/test/java/org/apache/seatunnel/connectors/seatunnel/socket/SocketFactoryTest.java:82-93`
> * Why this is a problem:
> the old Socket sink semantics allowed `max_retries = 0` and handled it
explicitly as “retry is disabled; fail immediately on the first write failure”.
The current head still rejects that value during config validation and removes
the runtime branch, so an existing user config can no longer be submitted at
all.
> * Risk:
> this is a real backward-compatibility break for existing Socket sink
jobs that intentionally use `max_retries = 0`.
> * Best fix:
>
> * Option A (recommended): keep the old contract and allow `max_retries
>= 0`, while restoring the fail-fast `maxNumRetries == 0` branch in
`SocketClient.write(...)`.
> * Option B: if the community intentionally wants to deprecate `0`, then
treat it as an explicit incompatible change and document the migration instead
of hiding it inside a validation-cleanup PR.
> * Severity: High
> * Raised by others already: No
>
> ### Issue 2: if this behavior change is intentional, the user-facing docs
and incompatible-change note are still missing
> * Location:
>
> * `docs/en/connectors/sink/Socket.md:27-30`
> * `docs/zh/connectors/sink/Socket.md:28-31`
> * Why this is a problem:
> the current Socket sink docs still do not say that `max_retries = 0` is
rejected, and there is still no incompatible-change record for that behavior
change.
> * Risk:
> users will keep following the current docs and only discover the change
when validation starts failing at submission time.
> * Best fix:
> if Issue 1 is fixed by restoring `0`, this problem disappears naturally.
Otherwise please update both docs and add an incompatible-change note.
> * Severity: Medium
> * Raised by others already: No
>
> ## Tests / stability
> * The added tests are structurally stable: no sleeps, no ports, no timing
dependency.
> * Stability rating: Stable.
> * The problem here is semantic correctness, not flakiness: the current
tests are asserting the wrong contract for `max_retries = 0`.
>
> ## CI note
> * The current GitHub state I saw on this latest head was:
>
> * `Notify test workflow`: pass
> * `labeler`: pass
> * `Build`: in progress
>
> The source-level compatibility blocker above is already enough for me
regardless of the final Build result.
>
> ### Merge conclusion: can merge after fixes
> 1. Blocking items
>
> * Issue 1: please preserve the historical `max_retries = 0` fail-fast
behavior, or explicitly handle it as a documented incompatible change.
>
> 2. Suggested non-blocking follow-up
>
> * Issue 2: if you intentionally keep the behavior change, also update both
the English and Chinese Socket sink docs and the incompatible-change note.
>
> Overall, I still agree with moving Socket sink validation into
`OptionRule`, but the current version still mixes that cleanup with a
user-visible contract change. Once the `max_retries = 0` compatibility issue is
resolved, the rest of the direction looks good.
@DanielLeens thanks for your review.
Before migration, if it is equal to 0, it will report an error during
runtime. It should be thrown to the configuration verification in advance, and
the user should be aware of it clearly. This is a fix for abnormal scenarios
and does not involve compatibility issues.
Before code:
``` java
if (maxNumRetries == 0) {
throw new SocketConnectorException(
SocketConnectorErrorCode.SEND_MESSAGE_TO_SOCKET_SERVER_FAILED,
String.format(
"Failed to send message '%s' to socket
server at %s:%d. Connection re-tries are not enabled.",
row, hostName, port),
e);
}
```
--
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]
