SEZ9 commented on PR #11932: URL: https://github.com/apache/seatunnel/pull/11932#issuecomment-5421603579
Thanks @DanielLeens for taking the time to re-verify against `4414b55b` rather than accepting the list at face value — that's exactly the kind of confirmation this needed, and I agree with your conclusions on all three points you checked. On the specific items: - **Issue 1 (empty-list guard)**: agreed, this needs an explicit `isEmpty()` check in `IncrementalSourceReader.initializedState()` so a non-null-but-empty `checkpointTables` list can't roll the deserializer/collector back to an empty schema. Not fixed yet. - **Issue 2 (restore asymmetry)**: agreed it's a real logic issue, not style. `restoreCheckpointProducedType(...)` should sit behind the same `getSchemaChangeResolver() != null` gate as the collector-side restore so the two can't diverge. Not fixed yet. - **Issue 5 / your Finding A**: your framing is the right one — these are two symptoms of the sink now unconditionally carrying state. I support the opt-in direction you sketched: only emit a non-null `JdbcSinkState` when schema evolution has actually occurred, so plain JDBC sinks keep returning an empty list from `snapshotState()` as before. That resolves Finding A at the root instead of patching the serializer gate or the error-sink lifecycle check separately. For Issues 3, 6, 7, and 8, I share your read: no counter-evidence, and given the hit rate on the sampled three, I'd like each of them either fixed or explicitly rebutted with evidence before this moves forward — in particular the non-deterministic first-non-null schema pick in `restoreWriter` and the silent insert-only downgrade when a restored schema lacks a `PrimaryKey`, since both are correctness issues on restore. The `Collector.restoreSchema` no-op on Flink/Spark also needs at least a documented limitation if not a fix, and the checkpoint state format change still needs a docs/upgrade note. So concretely, the remaining asks are: (1) empty-list guard, (2) resolver-gated deserializer restore, (3) opt-in state emission per the direction above, (4) responses or fixes for Issues 3/6/7/8, and (5) documentation of the state format change. Until those land, I agree the "not recommended for merge in current form" conclusion stands. <!-- streview-comment:568 --> -- 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]
