DanielLeens commented on PR #11677:
URL: https://github.com/apache/seatunnel/pull/11677#issuecomment-5229131118
@SEZ9 I re-checked this latest `CHANGES_REQUESTED` review against the
current head (`1684c382f`, unchanged since my last pass), and it restates the
exact same two findings, with the exact same wording, that you posted in your
prior review yesterday (2026-08-07T10:00:15Z) — which I already traced against
the actual diff and rebutted point-by-point in my review at
2026-08-07T11:15:54Z. Since the head hasn't moved, I re-verified both items
again from scratch just now and the conclusion is unchanged:
- **Issue 1 ("possible public API change", High/blocking):** I pulled the
file straight from the PR head and the only change to `addSplitsBack()` is:
```java
- public void addSplitsBack(List<SourceSplitBase> splits, int
subtaskId) {
+ public synchronized void addSplitsBack(List<SourceSplitBase> splits,
int subtaskId) {
```
plus the new `if (running) { assignSplits(); }` body and an explanatory
comment. Name, parameter list, and return type are byte-for-byte identical to
before; `synchronized` is a method modifier, not part of the Java method
signature (JLS 8.4.3.6), so it cannot break binary or source compatibility for
callers or overriders. I also re-ran a repo-wide search for any class extending
`IncrementalSourceEnumerator` and found zero results — there is no subclass in
the tree that overrides this method, so there's no override site that could
even theoretically be affected. I don't see a substantive basis for this being
a blocking API break; happy to be pointed at a specific caller/override that
would actually break if I'm missing something.
- **Issue 2 ("config/option changes without doc updates", Low):** I grepped
the full diff across all six changed files for any `Option`/config addition
(`new Option`, `.key(`, `.defaultValue(`, etc.) — there are none. This PR
touches zero configuration options anywhere, so there's nothing for docs to
reflect.
Since both items were already addressed with concrete evidence in the
previous round and nothing in the code has changed since then, I don't think
this restates a new blocker — it looks like the same review content was
reposted without picking up the rebuttal. I'd suggest resolving/withdrawing the
`CHANGES_REQUESTED` state unless there's a specific code path you can point to
that the above doesn't cover.
@li3zhi4 thanks for confirming convergence on the Issue 3 reachability note
and for updating the PR description accordingly — that resolves my only
remaining non-blocking concern from this side. The debug-log trace of the exact
partial-restart interleaving is a nice-to-have; no need to hold this PR for it,
a follow-up PR is fine.
With that, my own position is unchanged from my last review: the production
fix is minimal, correct, and backward-compatible, both prior test-stability
issues are genuinely resolved, and I don't see any code-level blocker
remaining. The only outstanding item on my side is confirming the required
`Build` check is green on the current head before merge.
--
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]
