SEZ9 commented on PR #11503: URL: https://github.com/apache/seatunnel/pull/11503#issuecomment-5650405509
Quick status update on this PR. On CI: the failures on the `e25c5c02ac3` build were traced individually in the triage comment above and none appear to be caused by this diff (image pull failures at container `startUp()` plus previously-tracked flaky tests), and CI has been retriggered. Once the rerun comes back we should have a cleaner signal. The review findings from the previous round are still open, and those are what's blocking from my side: - **F1 (HIGH)** – The legacy checkpoint restore path in `IncrementalSourceReader` hardcodes `default.default` as the tablePath, so the restored `CatalogTable`'s `TableId` can't match the real source table. Please derive the real table path from the checkpoint state (or explain how the id is reconciled downstream). - **F5** – The restore gate widened from `checkpointDataType != null` to any non-empty `checkpointTables`, so every restored CDC job now goes through the restore/event path even when no DDL ever occurred. Please either narrow the condition back or explain why the wider gate is intended and safe. - **F2** – `RestoreTableSchemaEvent` flows through the generic `SchemaChangeEvent` SPI; connectors/transforms not updated in this PR will hit unknown-event paths during failover recovery. Please list which sinks/transforms have been updated and what happens for the rest. - **F3 / F6** – In `AlterTableSchemaEventHandler` a null `changeAfter` silently falls through to stale-schema behavior, and the restore shortcut in `TableSchemaChangeEventDispatcher` replaces the current schema without verifying the event targets the same table. Please fail loudly on null and add a table-id check. - **F4** – `restoreCheckpointHistoryTableChanges` in `AbstractDebeziumDeserializationSchema` does a non-atomic `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 why single-threaded access is guaranteed. - **F7** – The new public SPI method `restoreCheckpointHistoryTableChanges` in `DebeziumDeserializationSchema` needs Javadoc. - **F8** – The full `CatalogTable` list (including options) is logged at INFO on every restore in `IncrementalSourceReader`; please downgrade to DEBUG and/or log only table ids. If any of these have already been addressed in a newer commit, please reply per item so I can re-check. Thanks! <!-- streview-comment:1006 --> -- 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]
