ryanmeowy commented on issue #10203: URL: https://github.com/apache/seatunnel/issues/10203#issuecomment-6567822057
@SEZ9 two things on the revision, neither of which needs anything from you if it reads right. **1. Your backlink suggestion is in, and it was needed.** The revision linked back to nothing — I checked and there were zero `issuecomment-` references in it, so §2 was a second independent restatement of the completion signal with no anchor. Both places now point at [6357660652](https://github.com/apache/seatunnel/issues/10203#issuecomment-6357660652), with §2 marked as a restatement for readability only and the table comment as authoritative if the two diverge. **2. Re-review found two traps in contract 1, now written in.** Both are in the admission path and both are the kind that pass a happy-path test: - **Admission must not be routed through `SnapshotSplitAssigner`.** `completedSnapshotPhase` opens with `checkArgument(isCompleted() && allSplitsCompleted())` (`SnapshotSplitAssigner:341-342`, predicate at `:299-301`). Nothing violates it today because no split enters that assigner after the snapshot phase ends; admission would be the first. T's `W_high` also raises `maxSnapshotSplitsHighWatermark`, so `markEnterPureIncrementPhaseIfNeed` fires a **second** `CompletedSnapshotPhaseEvent` once the stream passes it, and the enumerator forwards that unconditionally (`IncrementalSourceEnumerator:149-159`). Either half alone is survivable; the combination is not. - **The rebuilt streaming split must not be produced while the old one is still in `assignedSplits`.** `createIncrementalSplits` derives the new split's table set as `capturedTables − assignedSplits` (`IncrementalSplitAssigner:253-254`, map at `:87`), and nothing in that class ever removes from the map — it is written once at `:145`. Clear `splitAssigned` with the old entry still present and the rebuilt split carries `T` alone; `noMoreSplits()` then returns true and the old tables stop being read with no error. §7 items 4 and 5 are the regression tests. These were missing from the first posting, so I said so in §0 rather than editing them in silently. **3. Two statements were stale against #11206 head `d4faf1ea1`, so I updated rather than left them.** The `schema-changes.include` constraint in §5 was written against #11206's open Issue 3; that head closes it by making `CreateTableEvent` a control event that `SchemaChangeEventFilter` never filters, so the guard and its test case are gone, and §5 now records the one condition under which it comes back (if the bypass does not survive review and a canonical `create.table` name lands instead). §8.3 and §8.4 likewise record that `d4faf1ea1` took the re-admission shape §8.4 asked for — opt-in, replayed `CreateTableEvent`, checkpointed table added back to the split's `tableIds` — while flagging that those are the author's claims at that head, not yet your confirmation. That composition matters for this design: on that shape, admission only has to contribute `capturedTables` membership and the `tableStartupOffsets[T] = W_high` floor at restore. Full text is the edited comment above; the line numbers are still pinned to `dev` (re-checked at `b3db27900`, no drift in any cited file), not to the #11206 branch. -- 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]
