SEZ9 commented on PR #11677: URL: https://github.com/apache/seatunnel/pull/11677#issuecomment-6228695631
Thanks for pushing this through — good to see the `Build` run on head `2ba8abe87` (actions run 38893273138) come back green. That clears the CI side, but I'd still like to close out the points from the earlier review before this goes to the merge queue, since none of them were addressed in the thread yet: 1. **`SourceSplitEnumerator` javadoc vs. other connectors (F1/F2).** The rewritten javadoc now documents an ordering obligation for *all* enumerator implementations, but only `IncrementalSourceEnumerator` was hardened. Please either (a) narrow the javadoc so it describes the engine behaviour without imposing a new contract on every `addSplitsBack()` implementation, or (b) explicitly call out that the hardening is CDC-only and file a follow-up for the remaining connectors. Which direction do you prefer? 2. **Pre-run window in `addSplitsBack()` (F3/F4).** The `if (running)` gate means splits returned before `run()` executes are only dispatched via the trailing `assignSplits()` in `run()`. That invariant should at least be stated in a comment next to the gate, and ideally covered by a unit test that calls `addSplitsBack()` before `run()` and asserts the splits are still assigned. 3. **E2E timing in `AbstractMysqlCDCITBase` (F5/F6).** The 30-second wait for the injected sink failure to appear in the logs is much tighter than the 2-minute budgets elsewhere in the same test; please align it. Also, please confirm the `DROP TRIGGER` in the `finally` block is guaranteed to take effect before the restarted job replays the failing insert, so a capped retry budget can't be exhausted spuriously. 4. **`getServerLogs().substring(logOffset)` (F7/F8).** This can throw `StringIndexOutOfBoundsException` if the logs shrink/rotate during restart, which aborts the awaitility wait rather than retrying. A length guard (or searching the full log for the marker) would make it robust. Once those are addressed or explicitly deferred with a follow-up, I'm happy to take another look. A committer approval is still required for merge, and since the previous change request was recorded against the older head `1684c382f`, please re-request review once the above is pushed so the state can be refreshed. <!-- streview-comment:1814 --> -- 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]
