SEZ9 commented on PR #11677: URL: https://github.com/apache/seatunnel/pull/11677#issuecomment-6050945202
Thanks for the CI summary on `677216dd4f`. Good to see `mysql-cdc-connector-it` green on both JDK 8 and 11, since that is the module covering the recovery scenario here. I take your point that the remaining reds (`Dead links`, `all-connectors-it-2/6/7`, `engine-v2-it (8)`) are the same families seen on `dev`. A green run doesn't close out the earlier review points, though, since most of them aren't things CI would surface. Could you give a short status on each so I know what is addressed vs. deliberately left as-is? 1. **`SourceSplitEnumerator` javadoc** — the rewritten javadoc describes an ordering obligation that only the CDC base enumerator was updated to honor. Either narrow the wording so it stays a description of the engine-side guarantee rather than a new requirement on every `addSplitsBack()` implementation, or call out explicitly that other connectors are not yet conformant. Which direction are you taking? 2. **Pre-`run()` window behind the `if (running)` gate** — splits returned via `addSplitsBack()` before `run()` executes are only re-dispatched because of `run()`'s trailing `assignSplits()` call. That invariant should at least be documented in a comment next to the gate, and ideally covered by a unit test that calls `addSplitsBack()` before `run()` and asserts the splits get assigned. Is that on your list? 3. **30-second wait in `AbstractMysqlCDCITBase`** — the wait for the injected sink failure to appear in the engine logs is much tighter than the 2-minute budgets elsewhere in the same test. It passed on this head, but I'd still prefer it aligned with the other budgets to avoid a new flake source. Any objection to bumping it? 4. **Retry budget / trigger race** — if the restarted job replays the failing insert before the `finally` block's DROP TRIGGER takes effect, extra restart cycles get burned. Have you confirmed the retry budget has enough headroom, or made the failure injection one-shot? 5. **`getServerLogs().substring(logOffset)`** — this throws `StringIndexOutOfBoundsException` if the logs shrink or rotate during the restart window, which aborts the awaitility poll instead of retrying. A bounds check or clamping `logOffset` to the current length would make the assertion robust. Once these are addressed or explicitly deferred with a note, I'm happy to take another look. <!-- streview-comment:1588 --> -- 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]
