SEZ9 commented on PR #11503: URL: https://github.com/apache/seatunnel/pull/11503#issuecomment-5564238581
Thanks @DanielLeens for the fresh pass on `45cf6d03e3` and for the CI triage. Agreed that with `git diff 83d9cc3bde..45cf6d03e3 --stat` empty (the only commit being the empty `45cf6d03e` retrigger), there is no new source to re-trace, and I'm fine treating the `all-connectors-it-6 (11)` / `paimon-connector-it (8)` failures as outside this diff per your write-up, with the `DatabendCDCSinkIT#testDatabendSinkCDC` sighting kept on watch and the Paimon hang tracked in #12139. We'll see what the `3b26332a` retrigger produces. Since nothing in the source has moved since my previous review, all eight findings from that round are still open as far as this thread shows. Concrete asks before I can approve: - **PR11503-F1 (HIGH)** – the legacy restore path in `IncrementalSourceReader` still hardcodes tablePath `default.default`, so the restored `CatalogTable`'s `TableId` can't match the real source table. Please derive the real table path (or fail explicitly) instead of the placeholder. - **PR11503-F5** – the restore gate widened from `checkpointDataType != null` to any non-empty `checkpointTables`. Please confirm the intent and either narrow it back to "a DDL actually happened" or explain why every restored CDC job should go through the restore/event path. - **PR11503-F6** – the restore shortcut in `TableSchemaChangeEventDispatcher` should check the event's table identity before replacing the current schema. - **PR11503-F3** – a null `changeAfter` on `RestoreTableSchemaEvent` in `AlterTableSchemaEventHandler` should throw rather than fall through to the stale schema. - **PR11503-F4** – `restoreCheckpointHistoryTableChanges` in `AbstractDebeziumDeserializationSchema` does `clear()`+`putAll()` on a shared non-concurrent map; please make the swap atomic (e.g. build a new map and replace the reference) or document the single-threaded guarantee. - **PR11503-F2** – for connectors/transforms not touched in this PR, please state how `RestoreTableSchemaEvent` is handled on their unknown-event paths during failover (ignore vs. error), and add a test or note covering it. - **PR11503-F7** – Javadoc on the new `restoreCheckpointHistoryTableChanges` SPI method in `DebeziumDeserializationSchema`. - **PR11503-F8** – drop the INFO-level dump of the full `CatalogTable` list (including options) in `IncrementalSourceReader`; log table ids at INFO and the rest at DEBUG at most. If any of these were already addressed in a commit I'm not seeing referenced here, please point me to it and I'll re-check; otherwise a follow-up push covering at least F1, F5 and F6 is what I need to move this forward. <!-- streview-comment:854 --> -- 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]
