DanielLeens commented on PR #11932: URL: https://github.com/apache/seatunnel/pull/11932#issuecomment-6411498459
Thanks, @SEZ9 - your checklist is addressed to @davidzollo, but since I re-read this exact head (`f2ef35145d8f`, unchanged since my last review; nothing executed locally) I can share where each item lives, and where I could not find coverage. Paths are relative to the module `src/main` unless noted. 1. **Non-empty checkpoint/legacy table guard** - `IncrementalSourceReader.restoreCollectorCheckpointTables` (new static helper near line 300): returns empty when schema changes are disabled or `getSchemaChangeResolver()` is null, falls back to `restoreLegacyCheckpointTables` only when the modern list is null/empty, and `addSplits` only publishes the result when it is non-empty. Tests (`IncrementalSourceReaderTest`): `shouldIgnoreEmptyCheckpointTables:128`, `shouldRestoreLegacyCheckpointDataType:151`. 2. **Deserializer vs collector gating** - both use the same two conditions (schema changes enabled + non-null resolver); the collector side is consumed by `restoreCollectorSchema` at the top of `pollNext` (`getAndSet(null)`), i.e. before any restored record is emitted. Tests: `shouldRestoreCheckpointTablesOnlyForSchemaAwareDeserializer:103`, `shouldSkipRestoreForResolverlessDeserializer:181`, `shouldKeepLiveCollectorSchemaWhenSchemaChangeIsDisabled:204`. 3. **`restoreWriter` schema selection** - `JdbcSink.resolveRestoredTableSchema:133`: a single non-null schema wins, null (legacy) states are skipped, and two differing non-null schemas throw `IllegalStateException`. Tests in `JdbcSinkTest`: `shouldUseTheOnlyRestoredSchemaOrTheInitialSchema:40`, `shouldRejectDivergentRestoredSchemas:58`. 4. **Null-Xid filtering / `checkpointEnabled`** - `JdbcSink.createWriter` (private overload, ~line 326): XA writers receive only states with `getXid() != null`; the ordinary writer keeps `jobContext.isEnableCheckpoint()` as `checkpointEnabled` and passes `hasRestoredSchema` as a separate constructor flag. I could not find a direct unit test for the null-Xid filter or the flag separation; they are protected by code inspection and the MySQL restore IT only. 5. **Non-exactly-once writer state** - `JdbcSinkWriter.snapshotState` returns an empty list until `schemaEvolutionApplied` is set (set only after `applySchemaChange` succeeds, or by a restored schema). On the Serializable question: `JdbcSinkState` is serialized by `DefaultSerializer` and `JdbcSinkStateTest.testSerializeEvolvedTableSchema:43` round-trips a `TableSchema` built from `PhysicalColumn`s; `Column` and `PrimaryKey` are `Serializable`. I did not see a test with an exotic column payload, so that remains "covered for the standard column types". 6. **Primary key on a restored schema** - you are right that this is thin on tests. The behavior is in `JdbcSink.resolvePrimaryKeyIndex:366`: if the restored schema has no `PrimaryKey` it falls back to the configured one (with a WARN) and resolves the index against the restored column order; if neither exists it returns null. There is no direct unit test for this branch. 7. **Docs** - yes: `docs/en` and `docs/zh` `introduction/configuration/schema-evolution.md` gain a "Checkpoint restore compatibility" section covering the schema record, XA state, legacy fallback and the downgrade limitation. 8. **Flink/Spark no-op** - the `Collector.restoreSchema` Javadoc (`seatunnel-api`) states it is Zeta-only today, and the PR description links #12983. There is no comment at the `IncrementalSourceReader.restoreCollectorSchema` call site itself, so that half of your request is still open. For completeness, my last review (`#5644146114`) is still pending on the same head: Issue 3 (short Javadoc/comments for the new recovery contracts at `JdbcSink.java:134`, `AbstractJdbcSinkWriter.java:70` and `SeaTunnelSourceCollector.java:359`) has not been addressed yet, and the `Run / engine-v2-it (17, ubuntu-latest)` job is still red on this head. @davidzollo, items 4/6 (direct tests) and the call-site comment in item 8 would pair nicely with Issue 3 in a single small commit; I will re-review as soon as it lands. -- 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]
