DanielLeens commented on PR #11556: URL: https://github.com/apache/seatunnel/pull/11556#issuecomment-5369032586
@SEZ9 thanks for the fresh from-scratch pass — I re-checked Issue 1 against the current head (`13a093307ab5`) and it's a real, correctly-diagnosed gap that my own nine rounds missed. Confirming the call chain independently: - `SourceSplitEnumeratorTask.stateProcess()` calls `this.close()` on both `CLOSED` (normal completion) *and* `CANCELLING` (`SourceSplitEnumeratorTask.java`, the `CANCELLING: this.close(); currState = CANCELED;` branch) — so enumerator close is not a terminal-only event, it also fires on task cancellation, which is what a Zeta pipeline restart/failover drives. - `SourceSplitEnumeratorTask.close()` unconditionally calls `enumerator.close()`, which for the hybrid case reaches `HybridSplitAssigner.close()` → `snapshotSplitAssigner.close()` → `SnapshotSplitAssigner.close()` at `SnapshotSplitAssigner.java:277-279`, which itself unconditionally calls `dialect.closeEnumerator(sourceConfig)` — no terminal-vs-transient distinction at any layer of this chain. - `PostgresDialect.closeEnumerator()` (`PostgresDialect.java:174-215`) only skips the drop when `DROP_SLOT_ON_STOP` is false, or when the slot's `SlotState` is still `slotIsActive()` at that instant. It does not check whether the job is actually finishing versus being torn down for restart. During a failover the incremental readers are torn down around the same time as the enumerator, so there's a real window where the slot shows inactive and gets dropped — exactly the scenario you described, and it does directly threaten resume-from-checkpoint (the persistent slot's `confirmed_flush_lsn` is what the subsequent incremental split resumes from). So I'm updating my own conclusion: this needs to be added as a blocker alongside the CI-not-yet-green and #11847/#11864 items from my last round, not treated as a nice-to-have. Your suggested fix (gate the drop on a terminal condition, e.g. only from `SnapshotOnlySplitAssigner`/completed state, never from the `HybridSplitAssigner` snapshot leg while incremental is still the live path) looks like the right shape to me too. On the rest of the list: Issues 2 and 6 read as the same underlying finding (double-invoke of `closeEnumerator` on the `open()` failure path plus the framework's own `close()`) — worth folding into one fix. Issues 3–5 and 7 (slot-name charset validation, deterministic-name collision on crash/restart, byte- vs char-length truncation, and the `max_replication_slots` sizing note in the FAQ) all check out against the code as legitimate hardening gaps for exactly-once correctness, even if lower severity than Issue 1. Issue 8's try/finally point on `HybridSplitAssigner.close()` is a straightforward correctness fix. @davidzollo — given Issue 1 is a genuine data-loss-on-failover risk in code that's been stable across nine review rounds, I'd treat it as a must-fix alongside the two carried-forward blockers before this merges. -- 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]
