DanielCarter-stack commented on PR #10576:
URL: https://github.com/apache/seatunnel/pull/10576#issuecomment-4019293598
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10576", "part": 1,
"total": 1} -->
### BLOCKER: PR #10576 is completely duplicate of merged PR #10483
**Location**: `seatunnel-core/seatunnel-starter/src/main/bin/seatunnel.sh:47`
**Problem Description**:
By comparing the diffs of the two commits, it is confirmed that PR #10576
(981a08770) and PR #10483 (e00a58249) have identical code changes. Both commits
are based on the same ancestor (89b3e1ce) and independently created the same
fix on 2026-02-11. PR #10483 has already been merged into the dev branch with
the title '[Fix][Zeta] Preserve job name containing spaces in seatunnel.sh
(#10483)', while PR #10576 is a completely duplicate modification.
**Impact**:
If this PR is merged, it will create duplicate commits in the dev branch,
polluting the Git history and potentially causing merge conflicts. It wastes
review and maintenance resources. It is recommended to close this PR as the
issue has been completely resolved in PR #10483.
### MAJOR: Other startup scripts have the same parameter passing issue but
are not fixed
**Location**:
`seatunnel-core/seatunnel-starter/src/main/bin/seatunnel-cluster.sh:51`
**Problem Description**:
Both seatunnel-cluster.sh (line 55) and seatunnel-connector.sh (line 45) use
the same 'args=$@' pattern and have the same word splitting issue. However,
this PR only fixes seatunnel.sh and does not fix the other scripts. This leads
to inconsistent behavior among startup scripts in the project, and users will
still encounter the issue of parameters with spaces being truncated when using
seatunnel-cluster.sh.
**Impact**:
Affects the consistency of user experience. If the documentation states that
parameters support spaces, but only seatunnel.sh supports it, it will cause
confusion. It is recommended to create a separate PR or fix the other startup
scripts together in this PR to ensure consistent parameter handling behavior
across all scripts.
### MINOR: Lack of automated testing to verify parameter passing fix
**Location**: `seatunnel-core/seatunnel-starter/src/main/bin/seatunnel.sh:47`
**Problem Description**:
The PR fixes the parameter passing issue in the Shell script and provides
manual testing verification, but does not add automated tests to ensure this
issue will not regress. Although automated testing for Shell scripts is more
complex than for Java code, key scenarios can be covered through integration
tests.
**Impact**:
Future modifications may introduce the same bug without automatic detection
in CI/CD. It is recommended to add integration tests or Shell script tests to
verify scenarios including parameters with spaces, special characters, and edge
cases to prevent regression.
### MINOR: String matching detection for 'local' has theoretical edge cases
**Location**: `seatunnel-core/seatunnel-starter/src/main/bin/seatunnel.sh:86`
**Problem Description**:
When using 'args_str=" $* "' for string matching, if a parameter value
itself contains a substring like '-m local' (extremely unlikely), it may be
incorrectly judged as local mode. Although adding spaces before and after
avoids partial matching, there are still theoretical edge cases.
**Impact**:
This issue is almost impossible to occur in actual scenarios, as
configuration file paths or parameter values are unlikely to contain the exact
substring '-m local'. The current solution is sufficiently safe and does not
need modification, but it is worth documenting this behavior in code comments.
--
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]
