ryanmeowy commented on issue #10203: URL: https://github.com/apache/seatunnel/issues/10203#issuecomment-6649461321
@SEZ9 §7 items 4 and 5 are tightened in the design comment above; nothing else in it changed. One numbering note so the diff reads as intended: in §7 the table-set test is item 4 and the second-event test is item 5 — the reverse of the order in your ask — so I applied the `tableIds` failure condition to item 4 and the event condition to item 5. **Item 4** now fails if any pre-existing table is absent from the rebuilt split's `tableIds` (expected set, with the `incremental.parallelism = 1` the tests use: every pre-existing table plus T). That is the `capturedTables − assignedSplits` regression from §3 — it fails by silently shrinking the set, so a test that only asserts T is read passes on the broken build. **Item 5** now fails on three things: (a) `markEnterPureIncrementPhaseIfNeed` does not return true at T's `W_high`, so no event is emitted and the item is vacuous; (b) the event throws, at `SnapshotSplitAssigner.completedSnapshotPhase`'s `checkArgument(isCompleted() && allSplitsCompleted())` or `IncrementalSplitAssigner.completedSnapshotPhase`'s `checkArgument(splitAssigned && noMoreSplits())`; (c) the event changes the snapshot assigner's state — its `assignedSplits` and `splitCompletedOffsets` must still be empty and `isCompleted()` still true afterwards. "Either untouched or explicitly relaxed" is gone; untouched is the contract, and (c) is how the test fails if admission ever leaves bookkeeping there. One reading I want to close off, because your wording can be taken literally: §3 already says the second event is unavoidable — T's `W_high` becomes the split's new max watermark, so the emitter fires again once the rebuilt split reads past it — and a test that failed merely because a second event was emitted would fail against correct code. The three conditions above are what I read "fails if a second `CompletedSnapshotPhaseEvent` is observed after admission" to mean. If you meant the event should not exist at all (reader suppresses it, or the enumerator does not forward it to `SnapshotSplitAssigner`), that is a §3 change, not a §7 one; say which and I will take it there. For completeness on §5: I checked it against #11206's current head `c3d11fc6` (3 Nov, after our exchange). It changes no production code — the two commits are the rejected-fixture rename `95b4946ee` and the docs example `c3d11fc64` — so the `CreateTableEvent` bypass and the `restoreCheckpointProducedType` re-admission are as they were at `d4faf1ea1`, and §5 and §8.4 stand as written. Implementation stays queued per §8.3 until #11206's review closes. -- 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]
