DanielLeens commented on PR #11382:
URL: https://github.com/apache/seatunnel/pull/11382#issuecomment-5662011788
Thanks @SEZ9 — sorry about the apparent cut-off again. Since it happened
right at the F8 bullet both times now, I suspect it's something about how a
"show more"/collapsed-comment render interacts with a bullet that's followed
immediately by more bullets rather than a paragraph break, rather than anything
on GitHub's storage side (I re-pulled the raw stored body via the API again
just now and it's intact end-to-end). Rather than debug the renderer further,
here's F4-F7 restated on their own, short and separated, plus a genuinely new
item below that I want to flag as valid:
**F4 (drain-ready is process-memory only)** — open, tied to F2.
`schemaChangeDrainReady` is still guard-local in-memory state with no snapshot
of its own; the coordinator's replay of `latestCompletedCheckpoint` on restart
is what currently substitutes for it, but that dependency is only established
in review discussion, not stated in the source. Close together with F2's
javadoc fix.
**F5 (typed abort call site)** — resolved.
`SinkFlowLifeCycle.notifyCheckpointAborted(long, CheckpointType)`
(`SinkFlowLifeCycle.java:356`) calls
`schemaChangeDrainGuard.checkpointAborted(...)`, so the guard's abort-cleanup
path has a real, coordinator-driven caller.
**F6 (single-slot tracking)** — open, unchanged.
`schemaChangeBeforeCheckpointId` is still a single `long`; 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 exercises the real
`SinkFlowLifeCycle` -> `SeaTunnelTask` -> `CheckpointFinishedOperation`
serialization/dispatch round trip, and there's no schema-evolution-with-restart
e2e.
**On your F3 wire-format point — you're right, and I should have caught this
as its own item rather than folding it into F8.** I went back to
`CheckpointFinishedOperation.java` (current head `1ee22aced026`) and traced it
explicitly:
```java
protected void writeInternal(ObjectDataOutput out) throws IOException {
super.writeInternal(out);
out.writeLong(checkpointId);
out.writeBoolean(successful);
...
out.writeString(checkpointType.getName()); // new trailing field, :91
}
protected void readInternal(ObjectDataInput in) throws IOException {
super.readInternal(in);
checkpointId = in.readLong();
successful = in.readBoolean();
checkpointType = CheckpointType.fromName(in.readString()); //
unconditionally expects the field, :99
}
```
`readInternal` unconditionally reads the trailing `checkpointType` string
with no length/presence guard. In a mixed-version rolling upgrade — an
old-binary writer sends the pre-this-PR two-field wire format to a new-binary
reader — `readInternal` will attempt to read a string that was never written,
which fails the deserialization rather than degrading gracefully. That's a
real, unaddressed backward-compat gap distinct from F8 (F8 is about an
unrecognized *value* for a field that's present; this is about the field being
*absent* from an older peer's stream). Given this repo's priority on
rolling-upgrade compatibility for checkpoint/RPC wire formats, I'm elevating
this to its own tracked item and keeping it open alongside F2 as a blocker, not
folding it into F8 anymore. Thanks for pushing on it.
Net status, restated cleanly: **F2 (javadoc) and the wire-format
version-guard (formerly folded into F8, now its own item) are the two remaining
blockers; F4 rides with F2; F8 (value-side), F6, F7 are open non-blocking;
F1/F3(write-side)/F5 are resolved.**
On CI: this diff's head (`1ee22aced026`) still shows the apache-side `Build`
check as it was in my last comment — the one real failure in the fork run is
`CouchbaseIT` container bootstrap in an `all-connectors-it-6` shard, which
doesn't touch anything under this PR's files and matches a known recurring
flake on that shard; the `windows-latest` unit-test flakes I flagged earlier
have since cleared on their own. That doesn't change the merit-side conclusion
above.
--
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]
