SEZ9 commented on PR #11677: URL: https://github.com/apache/seatunnel/pull/11677#issuecomment-5903258002
Thanks for the status update and the rebase, @li3zhi4. Since the head is still `5ba2e3c518`, the points from the previous review remain open. Here is what I'd like to see before this moves forward: **`SourceSplitEnumerator` javadoc / contract** The rewritten javadoc documents a weaker engine-side ordering guarantee and a corresponding obligation for `addSplitsBack()` implementations, but only `IncrementalSourceEnumerator` was hardened. Please either (a) scope the wording to what the engine actually guarantees today and describe the CDC enumerator's stricter handling as implementation-specific, or (b) if a contract change for all enumerators is intended, state that explicitly in the PR description and open a follow-up for the remaining connectors. I'd prefer (a) for this PR. **`if (running)` gate and pre-run reassignment** Splits returned via `addSplitsBack()` before `run()` executes are only dispatched through the trailing `assignSplits()` call in `run()`. That works, but it's an implicit ordering invariant. Please add a short comment at both sites explaining why the pre-run path is safe, and extend `IncrementalSourceEnumeratorTest` with a case that calls `addSplitsBack()` before `run()` and asserts the splits are assigned once `run()` completes. **E2E robustness in `AbstractMysqlCDCITBase`** - Align the 30-second wait for the injected sink failure with the 2-minute budgets used elsewhere in the same test. - Guard `getServerLogs().substring(logOffset)` so a shorter/rotated log results in a retry (e.g. clamp the offset or treat it as "no match yet") instead of a `StringIndexOutOfBoundsException` that aborts the awaitility wait. - For the DROP TRIGGER / replay race: either ensure the trigger is dropped before the restart proceeds, or make the assertion tolerant of an extra restart cycle so a capped retry budget can't fail the test spuriously. Once those are pushed I'll take another look promptly. If you disagree with any of this — particularly the javadoc scoping — just say so here and we can settle it in-thread. <!-- streview-comment:1422 --> -- 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]
