DanielLeens commented on PR #11382: URL: https://github.com/apache/seatunnel/pull/11382#issuecomment-5652819722
@SEZ9 On the cut-off claim: this time I pulled the raw stored body for that comment directly via the REST API (`GET /repos/apache/seatunnel/issues/comments/5645087530`, not the rendered page), not just re-asserting it looked complete to me. It's 4843 characters and ends cleanly at "...F6/F7 open and unchanged. Same head, same gate — happy to re-review the moment a commit lands." There's no missing continuation on GitHub's stored side. I think this is the same "show more" rendering collapse we already hit twice on this thread, on both our ends — not an actual truncation. Rather than debug that further, here's the full F1-F8 status in one place, re-verified against the still-current head `1ee22aced026` (no new commit since 09-08): - **F1 (guard race)** — resolved. `SchemaChangeDrainGuard.checkpointCompleted(long, CheckpointType)` and `checkSchemaChangeCanApply()` are both `synchronized` on the same monitor, the wait is in a `while` loop with a single up-front deadline, so there's no missed-notification window and no spurious-wakeup budget reset. - **F2 (HIGH, the actual remaining blocker)** — open. The re-delivery mechanism itself is correct: an ordinary task-level restart keeps the same `CheckpointCoordinator` instance (`CheckpointManager.coordinatorMap` is populated once and never replaced), `restoreCoordinator(false)` resets `isAllTaskReady` to `false`, and the restarted task's own `READY_START` report re-triggers `allTaskReady()` -> `notifyCompleted(latestCompletedCheckpoint)`, which fans the typed `CheckpointFinishedOperation` out to the fresh guard. That's pinned by `CheckpointCoordinatorTest#testAllTaskReadyRoutesRestoredSchemaChangeCheckpointThroughNotifyCompleted`. What's still wrong is the javadoc on `SchemaChangeDrainGuard.checkpointCompleted` (`SchemaChangeDrainGuard.java:135-138`): it currently states the coordinator is "always recreated" on task restart, which isn't true for this exact restart shape, and that incorrect explanation hasn't been corrected yet. This is the one item I'd keep as a hard blocker. - **F3 (write-side null `checkpointType`)** — resolved. `CheckpointFinishedOperation.writeInternal` (`CheckpointFinishedOperation.java:82-91`) now fails fast with an `IOException` instead of an opaque NPE. - **F8 (read-side unknown type name)** — open. `readInternal` (`CheckpointFinishedOperation.java:99`) still calls the pre-existing `CheckpointType.fromName`, which throws an unchecked `IllegalArgumentException` with no catch for a name it doesn't recognize (e.g. version skew mid rolling-deploy). Non-blocking, but worth a defensive fallback. - **F4 (drain-ready is process-memory only)** — open, tied to F2. `schemaChangeDrainReady` is still guard-local memory with no snapshot; the coordinator's replay of `latestCompletedCheckpoint` is what substitutes for that today, but that dependency is only established in this comment thread, not in the source. I'd close F4 together with F2 once the javadoc states that invariant explicitly. - **F5 (typed abort call site)** — resolved. `SinkFlowLifeCycle.notifyCheckpointAborted(long, CheckpointType)` (`SinkFlowLifeCycle.java:356`) now calls `schemaChangeDrainGuard.checkpointAborted(...)`, so the guard's abort-cleanup path is reachable from a real coordinator-driven sender. - **F6 (single-slot tracking)** — open, unchanged. `schemaChangeBeforeCheckpointId` is still a single `long`, so two overlapping schema-change-before/after pairs still can't be distinguished. - **F7 (wiring/e2e coverage)** — open, unchanged. Current tests exercise the guard directly or stub `notifyCompleted`; nothing goes through the real `SinkFlowLifeCycle` -> `SeaTunnelTask` -> `CheckpointFinishedOperation` serialization/dispatch round trip, and there's no schema-evolution-with-restart e2e case. Net: F2's javadoc fix is the actual remaining blocker; F4 rides along with it; F8/F6/F7 are non-blocking follow-ups. On CI, since I was asked to factor in the live state: the apache-side `Build` check is still `failure` on this same head. I dereferenced it to the real fork run (`davidzollo/seatunnel` run `34186793225`) and pulled the job logs rather than going off the summary. The only genuine test failure in that run is `org.apache.seatunnel.e2e.connector.couchbase.CouchbaseIT` (`Tests run: 1, Errors: 1`) failing during container bootstrap in `all-connectors-it-6 (11, ubuntu-latest)` — this diff doesn't touch anything under the Couchbase connector, and it matches a known recurring container-bootstrap flake on that IT shard. The paired `all-connectors-it-6 (8, ubuntu-latest)` shard shows `cancelled`, consistent with this workflow's fail-fast behavior after its `(11)` sibling failed, not a second independent break. For what it's worth, the `unit-test (11/8, windows-latest)` failures I flagged on the 09-11 run are green on this same head now, so that particular flake cleared on its own. None of this changes the merge gate story: F2 is still the real blocker, and a rerun of the Couchbase shard is a separate, unrelated action whenever davidzollo wants to clear it. -- 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]
