DanielLeens commented on PR #11677: URL: https://github.com/apache/seatunnel/pull/11677#issuecomment-6116668331
Hi @SEZ9, thanks for re-checking against the current head. I traced each point on `8b430e980` (I also posted a full re-review on this head earlier, which diffed the PR-owned files against `677216dd4` and found them identical). Pointers: **F1/F2, Javadoc scope.** `SourceSplitEnumerator.java` (the `run()` Javadoc, around lines 41-57) now reads "Methods are typically called in this order..." and says the engine only guarantees `run()` after `open()` and after every reader has registered. It states that `addSplitsBack()` may arrive before or after `run()`, and that dispatch of late splits is left to the implementation. It no longer promises a fixed, non-concurrent order, so it adds no new obligation on other connectors. The stricter behavior is documented only in the class Javadoc of `IncrementalSourceEnumerator`. **F3/F4, `if (running)` gate and pre-run window.** Comments are at both sites in `IncrementalSourceEnumerator`: the pre-`assignSplits()` block in `run()` and the block above `if (running)` in `addSplitsBack()`. `addSplitsBack()` is now `synchronized`, the same monitor as `run()` and `handleSplitRequest()`, so the `running` check cannot race with `run()` setting it. **Exactly-once.** `IncrementalSourceEnumeratorTest#shouldAssignSplitsAddedBackBeforeRunExactlyOnce` does `handleSplitRequest(0)`, then `addSplitsBack(...)`, then asserts nothing is assigned yet (no early dispatch while `running == false`). After `run()` it asserts each of `splitAssigner.restoredSplits`, `context.assignedSplits` and `context.allAssignedSplits` equals the singleton list of the restored split. Because `allAssignedSplits` is cumulative, a double assignment would fail it and a drop would fail it too. **F5-F8, `AbstractMysqlCDCITBase`.** - F5: the failure-surface wait is `await().atMost(2, TimeUnit.MINUTES)` (about line 265), aligned with the surrounding 2-minute budgets. - F7/F8: the log slice is `substring(Math.min(logOffset, serverLogs.length()))` (about line 275), so a shorter or rotated log makes the poll retry instead of throwing `StringIndexOutOfBoundsException`. - F6: `DROP TRIGGER` in the `finally` block is not strictly ordered before a replay, and I would not claim a hard guarantee. The test drops the trigger as soon as the failure is seen, and a restart replaying the failing insert first would consume one retry. `mysqlcdc_to_mysql_with_sink_failure_recovery.conf` sets `job.retry.times = 3` with a 15-second interval, and the 15-second wait before the first restart normally covers the drop. So the tail is bounded by retry headroom rather than eliminated. That seems an acceptable trade-off to me. One caveat: the real Docker/Zeta E2E pass the author reported was on `677216dd4`, and it has not been re-run on this merge head. The PR-owned files are identical, but Build on `8b430e980` is still waiting on a fork Actions run, which a maintainer may need to approve. I have no remaining code-level concern from my side. -- 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]
