DanielLeens commented on PR #11677: URL: https://github.com/apache/seatunnel/pull/11677#issuecomment-6210067357
Thanks @SEZ9 for the careful follow-up, and @li3zhi4 for the quick turnarounds. I re-read the four open items against the current head `c959fd198` so the author has one place to see where each stands. This is a thread reply, not a new review: the head has not changed since my last review, and my technical conclusion there is unchanged. **Items 1 / 2 (`SourceSplitEnumerator` javadoc, F1/F2):** the javadoc was already narrowed on this head. `SourceSplitEnumerator.java:42-57` now says the engine only guarantees that `run()` is invoked after `open()` and after every reader has registered, that the listed order is "typically" followed, and that how late `addSplitsBack()` splits are dispatched "is left to the implementation". The CDC-specific behavior is described in the class javadoc of `IncrementalSourceEnumerator.java:38-48`. That is the "narrow the javadoc" option @SEZ9 said is fine, so I read these two as satisfied; @SEZ9, please confirm when you do your pass. **Items 3 / 4 (`if (running)` gate, F3/F4):** the invariant is documented where the gate is: the comment block in `IncrementalSourceEnumerator.java:101-111` explains that with `running == false` the returned splits stay queued and are dispatched by the first `assignSplits()` pass in `run()`, and `run()` carries the matching comment at `:77-83`. The "addSplitsBack before run" path is also covered by a unit test: `IncrementalSourceEnumeratorTest#shouldAssignSplitsAddedBackBeforeRunExactlyOnce` (`IncrementalSourceEnumeratorTest.java:60-80`) asserts that nothing is dispatched while `running == false` (line 73) and that the split is assigned exactly once after `run()`. **Item 5 (30-second wait, F5):** I could not find a 30-second wait for the injected sink failure on this head. The failure-observed wait in `AbstractMysqlCDCITBase.java` uses `atMost(2, TimeUnit.MINUTES)` (the Awaitility block around line 265), the same budget as the convergence waits. The only 30-second wait is the check that the trigger is gone (`information_schema.triggers` count equals 0), which is a single catalog query rather than something waiting on the engine. If you were looking at an older head, this one is already aligned; if you meant that 30s check specifically, @SEZ9, let us know and @li3zhi4 can widen it, but I do not see it as a stability risk. **Item 6 (replay before `DROP TRIGGER`, F6):** this is explained in the test comment (`AbstractMysqlCDCITBase.java`, the block above the Awaitility wait) and in `mysqlcdc_to_mysql_with_sink_failure_recovery.conf:18-26`: the trigger is dropped in `finally` as soon as the failure shows up in the engine log, and if the restart wins that race the replay fails once more and consumes one of the extra retries from `job.retry.times = 3`. I checked that the budget is large enough (3 retries, 15s interval); this is a bounded one-extra-failure window, not an open-ended one. **CI, since the facts changed since my last CI note:** the head is unchanged, `Build` is still red, and the failing jobs are still the two `CassandraIT` cases on the Flink 1.20 container, which this PR cannot reach. The fix #12756 is already on `dev`, and the branch is now 27 commits behind `dev` and diverged. @li3zhi4, the lowest-cost next step is still to sync with the latest `dev` and let CI run again; if `CassandraIT` still fails after that, please paste the new failing check link here and I will help narrow it down. I do not see any remaining source-level blocker on this head. The remaining gates are a green `Build` and the human approval from @SEZ9 / a write-enabled maintainer. Thanks again for the patience on a tricky restart-path fix! -- 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]
