DanielLeens commented on PR #11932: URL: https://github.com/apache/seatunnel/pull/11932#issuecomment-5391637649
Thanks @SEZ9 — these eight issues are all distinct from my own Findings A and B, not overlapping restatements, so I re-verified a representative sample directly against the current head (`4414b55b`) rather than taking the list at face value: - **Issue 1 (empty-list guard)** — confirmed. `IncrementalSourceReader.initializedState()` now guards only on `incrementalSplit.getCheckpointTables() != null`, with no `isEmpty()` check, where the previous `checkpointDataType != null` guard implicitly covered this. Real gap, agreed. - **Issue 2 (deserializer restore unconditional vs. collector restore resolver-gated)** — confirmed at the same location: `restoreCheckpointProducedType(...)` runs before the `if (getSchemaChangeResolver() != null)` gate that protects `restoredCheckpointTables`. Agreed this is a real asymmetry, not just a style nit — it's the same class of mismatch this PR exists to close, just moved one level down. - **Issue 5 (non-exactly-once writer now always serializes `TableSchema`)** — confirmed: `JdbcSinkWriter.snapshotState()` unconditionally returns `Collections.singletonList(new JdbcSinkState(null, tableSchema))` where it previously returned an empty list. This interacts directly with my own Finding A: making writer state non-empty is exactly what causes `getWriterStateSerializer()` to need to be non-empty too, which is what trips the error-sink lifecycle gate in `DefaultErrorSinkWriter`. So Issue 5 and my Finding A are two symptoms of the same root design tension (this sink now always carries state), not independent problems — worth fixing together rather than patching each symptom separately. I haven't independently re-derived Issues 3, 6, 7, and 8 line-by-line, but I have no counter-evidence against any of them, and the pattern of the three I did check (all accurate, all real) gives me no reason to doubt the rest. Given the combined list — my Findings A and B, plus your Issues 1/2/5 confirmed and 3/6/7/8 provisionally accepted — my "not recommended for merge in current form" conclusion stands and the blocker set is larger than either of us captured alone. In particular, Issue 5 changes the shape of the right fix for my Finding A: rather than choosing between "gate the serializer on exactly-once" or "relax the error-sink lifecycle check," the schema-persistence behavior probably needs to be opt-in (only emit non-null state when schema evolution actually occurred) so that (a) plain JDBC sinks keep checkpointing an empty list like before, (b) error-routed JDBC sinks stay compatible with `DefaultErrorSinkWriter`, and (c) exactly-once sinks still get their Xid round-tripped. That's a design-level answer to both Finding A and Issue 5 at once rather than two separate patches. @davidzollo — given the number of open items across both reviews and the still-diverging relationship with #11780, I'd suggest reconciling into one branch before the next round of fixes rather than patching this head incrementally; happy to do a full fresh pass once that's settled. -- 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]
