DanielLeens commented on PR #11382: URL: https://github.com/apache/seatunnel/pull/11382#issuecomment-5633079778
@SEZ9 Good question — let me point to the actual commit and diff rather than re-describing it, since you're right that prose isn't verifiable. **Commit**: the head hasn't moved since davidzollo's push on 09-08 (`1ee22aced026`) — I re-confirmed that's still the current head as of this reply. That's the commit for everything below. **F1 (guard race)** — confirmed as described, at that exact head: - `SchemaChangeDrainGuard.checkpointCompleted(long, CheckpointType)` (`SchemaChangeDrainGuard.java:143-160`) is `synchronized`, sets `schemaChangeDrainReady = true` and calls `notifyAll()` on the qualifying branch. - `checkSchemaChangeCanApply()` (`SchemaChangeDrainGuard.java:193` onward) is the paired `synchronized` wait loop (`while (!schemaChangeDrainReady) wait(...)`), bounded as described. This matches the fix shape you outlined — I'd treat F1 as resolved at this head. **F2 (re-delivery path / javadoc)** — partially, not fully. The test is there: `CheckpointCoordinatorTest.testAllTaskReadyRoutesRestoredSchemaChangeCheckpointThroughNotifyCompleted` pins down `allTaskReady()` -> `notifyCompleted(latestCompletedCheckpoint)` with the type intact. But the javadoc fix I asked davidzollo for on 09-10 has **not actually been pushed yet**. `SchemaChangeDrainGuard.checkpointCompleted`'s javadoc (`SchemaChangeDrainGuard.java:135-138`) still reads: > "Since the pipeline's `CheckpointCoordinator` is always recreated together with a restarting task (task failure recovery is pipeline-scoped, not per-task, in this engine -- see `SubPlan.cancelPipeline()`/`reset()`), `isAllTaskReady`'s once-per-coordinator guard can never suppress this replay for a task that actually needs it." That's the exact sentence I flagged as wrong — the coordinator survives an ordinary task-level restart (`SubPlan.prepareRestorePipeline()` looks the existing coordinator up from `CheckpointManager.coordinatorMap`, it isn't reconstructed). What actually re-arms the replay is `restoreCoordinator(false)`'s `isAllTaskReady.set(false)`, not coordinator recreation. So please keep F2 open for now — the runtime mechanism is right, but the shipped explanation in the comment is still the old, inaccurate one, and I don't want that merged as the documented reasoning. **F3/F8 (wire format)** — half-addressed: - Write side: `CheckpointFinishedOperation.writeInternal` (`CheckpointFinishedOperation.java:82-91`) now fails fast with an `IOException` if `checkpointType` is null, instead of an opaque NPE from `.getName()`. That covers the null-write risk, via fail-fast rather than a sentinel/null-safe encoding — I'm fine with that as the fix shape. - Read side: `readInternal` (`CheckpointFinishedOperation.java:99`) does `checkpointType = CheckpointType.fromName(in.readString())`. `CheckpointType.fromName` is pre-existing and untouched by this PR, and it throws an unchecked `IllegalArgumentException` for any name it doesn't recognize — no catch, no fallback. That part is still open: an unrecognized type name on the wire (for example a version skew during a rolling deploy, since this operation now carries a brand-new field) will blow up the operation-processing thread instead of degrading gracefully. I'd like to see that handled defensively before merge. **F4 (drain-ready is process-memory only)** — no snapshot/state changes were added; `schemaChangeDrainReady` is still guard-local memory, not part of the sink's checkpoint state. The implicit position, based on the F2 discussion, is that the coordinator's replay of `latestCompletedCheckpoint` substitutes for actually snapshotting the flag. I'm fine closing F4 together with F2 **once** the F2 javadoc correctly documents that invariant — right now that dependency is only established in this PR-comment thread, not in the code, so a future change to `allTaskReady()` has nothing in the source telling it this behavior is load-bearing for schema-change recovery. Until the javadoc lands, I'd rather keep F4 open alongside F2 than close it on the strength of this conversation alone. **F5 (typed abort call site)** — this one is now wired, marking it resolved: `SinkFlowLifeCycle.notifyCheckpointAborted(long, CheckpointType)` (`SinkFlowLifeCycle.java:356`) calls `schemaChangeDrainGuard.checkpointAborted(checkpointId, checkpointType)`, fed by the same typed `CheckpointFinishedOperation(successful=false, checkpointType)` path. There's a real coordinator-side sender now, not an unreachable cleanup method. **F6 (single-slot tracking)** — still open, unchanged. `schemaChangeBeforeCheckpointId` (`SchemaChangeDrainGuard.java:60`) remains a single `long` field, so a second schema-change-before checkpoint starting before the first one's AFTER checkpoint completes still isn't distinctly tracked — the guard has no way to tell which in-flight schema change a given completion notification actually belongs to. **F7 (wiring/e2e test)** — still open. This head's test additions are `CheckpointCoordinatorTest.testAllTaskReadyRoutesRestoredSchemaChangeCheckpointThroughNotifyCompleted` (coordinator-only, `notifyCompleted` is stubbed) and `SchemaChangeDrainGuardTest` (guard exercised in isolation). Neither goes through the actual `SinkFlowLifeCycle` -> `SeaTunnelTask` -> `CheckpointFinishedOperation` round trip, and there's no e2e config added in this diff for a schema-evolution-with-restart scenario. I'd still like at least one test that goes through the real operation serialization/dispatch path rather than calling the guard directly. So, current status at `1ee22aced026`: F1 and F5 resolved; F2 and F4 tied together and still open pending the javadoc correction; F3 resolved (write side), F8 still open (read side); F6 and F7 unchanged and still open. Happy to re-review as soon as the javadoc fix and the remaining items land — that just needs the commit, not another prose explanation. Separately, for visibility: I also checked the current CI state on this head. `Build` is failing, but the two failing jobs are `unit-test (11, windows-latest)` and `all-connectors-it-6 (11, ubuntu-latest)`, which look like environmental flakes (Hazelcast node-startup timing on Windows, IT container timing) rather than anything tied to this diff. I wouldn't spend time chasing that right now — the source-side items above are the actual gate. -- 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]
