DanielCarter-stack commented on PR #10633:
URL: https://github.com/apache/seatunnel/pull/10633#issuecomment-4103255859
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10633", "part": 1,
"total": 1} -->
### Issue 1: PR Title Severely Mismatches Content
**Location**: PR Title vs Actual Changed Files
**Problem Description**:
- PR title claims to only modify "website build timeout"
- Actually includes modifications to 3 files, 2 of which are unrelated to
the title
- This is a **serious violation of PR standards**
**Potential Risks**:
- Risk 1: Reviewers may only focus on title-related content, ignoring other
changes
- Risk 2: Merging changes that should not be merged (such as .asf.yaml)
- Risk 3: Code review process is compromised
**Impact Scope**:
- Direct Impact: All 3 modified files
- Indirect Impact: May mislead future maintainers
- Affected Area: Entire project's PR standards
**Severity**: **CRITICAL**
**Improvement Suggestions**:
```markdown
# Suggest splitting into 3 independent PRs:
## PR 1: [Docs][API] Add JavaDoc to MultiTableWriterRunnable
- 只包含 seatunnel-api/.../MultiTableWriterRunnable.java 的变更
- 标题准确反映内容
## PR 2: [Chore][CI] Increase website build timeout to 90 minutes
- 只包含 .github/workflows/backend.yml 的变更
- 但需要先检查 dev 分支是否已有此修改（PR #10602）
## PR 3: [Chore] Update collaborators order in .asf.yaml
- 只包含 .asf.yaml 的变更
- 需要说明为什么需要调整顺序
- 或者直接回退这个变更（如果是意外修改）
```
**Rationale**:
- Each PR should have a clear, single purpose
- Facilitates code review and rollback
- Aligns with Apache project best practices
---
### Issue 2: Duplicate Commit - Website Timeout Modification Already Exists
in dev Branch
**Location**: `.github/workflows/backend.yml:330`
**Related Context**:
- PR #10602 already merged to dev (commit a0cefad8f)
- Current PR's commit 772f90a0f makes **exactly the same modification**
**Problem Description**:
- Current PR modifies workflow timeout from 60 to 90 minutes
- But this modification was already completed and merged in PR #10602
- If current PR is merged, it may cause conflicts
**Potential Risks**:
- Risk 1: Git conflicts (same lines modified twice)
- Risk 2: Wasting time reviewing already merged code
- Risk 3: Compromising CI history records
**Impact Scope**:
- Direct Impact: .github/workflows/backend.yml
- Indirect Impact: GitHub Actions execution
- Affected Area: CI/CD process
**Severity**: **MAJOR**
**Improvement Suggestions**:
```bash
# 1. Remove backend.yml modifications from the current PR
git checkout dev -- .github/workflows/backend.yml
# 2. Resubmit with only JavaDoc changes
git add
seatunnel-api/src/main/java/org/apache/seatunnel/api/sink/multitablesink/MultiTableWriterRunnable.java
git commit -m "[Docs][API] Add JavaDoc to MultiTableWriterRunnable"
# 3. Or, if all changes must be kept, you should:
# - Clearly explain in the PR description why duplicate modifications are
needed
# - But more likely: this should be removed
```
**Rationale**:
- Avoid duplicate work
- Keep Git history clean
- Prevent merge conflicts
---
### Issue 3: Modification to .asf.yaml Lacks Explanation and Justification
**Location**: `.asf.yaml:36-38`
**Related Context**:
- Commit f8a5697ac unexpectedly includes this modification
- That commit's title is "[Docs][Core] Add Javadoc to
MultiTableWriterRunnable"
- No related Issue or PR discussion
**Problem Description**:
- `.asf.yaml` is a critical configuration file for ASF projects
- Changing collaborators order should have a clear reason
- Current PR provides no explanation
**Potential Risks**:
- Risk 1: Accidentally modified repository permission configuration
- Risk 2: May be developer's local configuration accidentally committed
- Risk 3: Violates ASF project governance standards
**Impact Scope**:
- Direct Impact: GitHub repository's collaborator list
- Indirect Impact: Perceived order of collaborators (though no functional
impact)
- Affected Area: Project management
**Severity**: **MAJOR**
**Improvement Suggestions**:
```bash
# Option 1: Revert this change (recommended)
git checkout dev -- .asf.yaml
git commit -m "Revert accidental .asf.yaml change"
# Option 2: If it must be kept, you should:
# 1. Create a separate PR
# 2. Explain in the PR description why the order needs to be adjusted
# 3. Get PMC approval
```
**Rationale**:
- `.asf.yaml` should not be modified in a "JavaDoc addition" PR
- Any configuration changes should have clear reasons and review
- Complies with ASF project governance requirements
---
### Issue 4: JavaDoc Format Inconsistent
**Location**: `MultiTableWriterRunnable.java:52-64`
**Related Context**:
- Commit history shows JavaDoc underwent multiple modifications
- 16e194976: First addition
- f71379a64: Applied @davidzollo's suggestions
- 6ed1c395b: Formatting
**Problem Description**:
- Current HEAD's JavaDoc is the formatted version (6ed1c395b)
- But dev branch may not have this formatting yet
- May cause merge conflicts
**Potential Risks**:
- Risk 1: Manual conflict resolution required during merge
- Risk 2: Format inconsistency makes code review difficult
**Impact Scope**:
- Direct Impact: MultiTableWriterRunnable.java
- Indirect Impact: Code readability
- Affected Area: Single class
**Severity**: **MINOR**
**Improvement Suggestions**:
```bash
# Ensure the PR is based on the latest dev branch
git fetch origin
git rebase origin/dev
# Or, explain in the PR description:
# "This PR includes JavaDoc formatting improvements from commit 6ed1c395b"
```
**Rationale**:
- Maintain consistency with main branch
- Reduce merge conflicts
- Improve code quality
---
--
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]
