DanielLeens commented on issue #10203: URL: https://github.com/apache/seatunnel/issues/10203#issuecomment-6704297207
Thanks for tightening the two regression assertions. Your reading of the remaining ambiguity is right: the important invariant is not that a second `CompletedSnapshotPhaseEvent` must never be emitted. With an admitted table whose `W_high` becomes the new maximum, that event can be produced again. The required boundary is where that later event is handled. On current `dev`, `IncrementalSourceEnumerator.handleSourceEvent(...)` forwards every `CompletedSnapshotPhaseEvent` to `HybridSplitAssigner.completedSnapshotPhase(...)`; that method invokes both `SnapshotSplitAssigner.completedSnapshotPhase(...)` and `IncrementalSplitAssigner.completedSnapshotPhase(...)`. That is not a safe contract for post-admission completion: the snapshot assigner is for the original snapshot phase, while the incremental assigner has the strict `splitAssigned && noMoreSplits()` precondition and retains its previously assigned split set. Please therefore state the rule this way in the design: 1. A post-admission completion event is allowed and must be handled deterministically; do not make the test fail merely because the reader emitted it. 2. It must take a runtime-admission-specific completion path (a dedicated event, or an explicit phase-aware branch in the enumerator). That path must not re-enter `SnapshotSplitAssigner.completedSnapshotPhase(...)` after the original snapshot transition. 3. The re-armed incremental split must contain the prior incremental table set plus `T`. It must not be rebuilt from `capturedTables - assignedSplits` while the old incremental assignment is still retained, because that drops the pre-existing tables without an error. 4. The regression test should drive the reader past `T`'s `W_high`, then assert that handling the resulting event neither throws nor mutates the completed original-snapshot bookkeeping, and that the rebuilt split still contains every original table and `T`. This resolves the wording in my earlier review: the event itself is expected; routing it through the original snapshot-phase cleanup is not. Please keep the implementation queued until the prerequisite runtime-discovery work in #11206 has its current-head acceptance gates settled, then make this a separate, focused follow-up rather than mixing the two state-machine changes. -- 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]
