DanielCarter-stack commented on PR #10511:
URL: https://github.com/apache/seatunnel/pull/10511#issuecomment-3957033952
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10511", "part": 1,
"total": 1} -->
### Issue 1: PR contains unrelated seatunnel.sh modifications
**Location**: `seatunnel-core/seatunnel-starter/src/main/bin/seatunnel.sh`
**Issue Description**:
The title and description of this PR both state it is "Fix the misleading
docker-compose.yaml demo", which is a documentation fix. However, the git diff
shows that `HEAD~2..HEAD` includes modifications to `seatunnel.sh`.
Analysis of the commit history reveals:
- `HEAD` (46af1c480): [Docs] Fix the misleading docker-compose.yaml demo
(#10496)
- `HEAD~1` (e00a58249): [Fix][Zeta] Preserve job name containing spaces in
seatunnel.sh (#10483)
This indicates that the direct parent commit of the current HEAD is another
independent PR (#10483), which fixes the issue of handling job names with
spaces in seatunnel.sh. However, this modification is completely unrelated to
the current docker-compose.yaml documentation fix.
**Potential Risks**:
- PR scope is unclear, violating the "one PR does one thing" principle
- Code reviewers need to understand two unrelated changes
- If this PR is merged, the seatunnel.sh modifications will be merged
together, but it has no separate review record on GitHub
- From the GitHub PR page, the seatunnel.sh modifications may not be visible
(depending on how GitHub displays PRs that include parent commit changes)
**Impact Scope**:
- Direct impact: Behavior of the seatunnel.sh script
- Indirect impact: All users using the SeaTunnel command line
- Affected area: Core startup script
**Severity**: MAJOR
**Improvement Suggestions**:
These two changes should be separated into two independent PRs:
1. PR #10496: Contains only documentation modifications
2. PR #10483: Contains only seatunnel.sh modifications (this PR should
already exist independently)
If PR #10483 has not been merged yet, the author should:
1. Rebase the current PR against the latest dev branch (without the
seatunnel.sh modifications)
2. Or close this PR and create a new PR containing only the documentation
modifications
**Confidence**: High - Git history clearly shows these are two separate
commits
### Issue 2: Example comments in documentation may confuse users
**Location**: `docs/en/getting-started/docker/docker.md:352`
**Issue Description**:
In the cluster expansion example, the configuration item has a comment:
```yaml
-
ST_DOCKER_MEMBER_LIST=172.16.0.2:5801,172.16.0.3:5801,172.16.0.4:5801,172.16.0.5:5801
# add ip to here
```
This comment says "add ip to here", but in reality users need to add the
complete `ip:port` format, not just an IP. If users follow the comment and only
add an IP (such as `172.16.0.5`), the configuration will error.
**Potential Risks**:
- Users may misunderstand the comment and only add an IP without adding the
port
- This will cause the cluster to fail to work properly after expanding nodes
**Impact Scope**:
- Direct impact: Users attempting to expand their cluster following the
documentation
- Indirect impact: Cluster deployment failure
- Affected area: Docker Compose cluster deployment users
**Severity**: MINOR
**Improvement Suggestions**:
Modify the comment to a more accurate description:
```yaml
-
ST_DOCKER_MEMBER_LIST=172.16.0.2:5801,172.16.0.3:5801,172.16.0.4:5801,172.16.0.5:5801
# add new member address (ip:port) to here
```
Or explicitly state in the documentation that the complete `ip:port` address
needs to be added.
**Confidence**: Medium - This is a potential confusion point, but not a
serious error
### Issue 3: Inconsistent documentation comments between Chinese and English
versions
**Location**:
- `docs/en/getting-started/docker/docker.md:352`
- `docs/zh/getting-started/docker/docker.md:345`
**Issue Description**:
The comment at the same location in the Chinese documentation is:
```yaml
-
ST_DOCKER_MEMBER_LIST=172.16.0.2:5801,172.16.0.3:5801,172.16.0.4:5801,172.16.0.5:5801
# 添加ip到这里
```
The English comment is `# add ip to here`, and the Chinese comment is `#
添加ip到这里`. Both only say "add ip" without explaining that the complete address
(including port) needs to be added.
**Potential Risks**:
- Both Chinese and English documentation have the same potential confusion
issue
- Users may only add an IP and forget the port
**Impact Scope**:
- Direct impact: Users reading the documentation and attempting to expand
the cluster
- Indirect impact: Configuration errors causing cluster formation failure
- Affected area: All users using Docker Compose to expand clusters
**Severity**: MINOR
**Improvement Suggestions**:
Update the comments in both Chinese and English documentation simultaneously
to make them more accurate:
- English: `# add new member address (ip:port) here`
- Chinese: `# 在此处添加新成员地址`
**Confidence**: Medium - The comment is not precise enough, but not a
serious issue
--
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]
