SEZ9 commented on PR #11503: URL: https://github.com/apache/seatunnel/pull/11503#issuecomment-5578420689
Thanks @DanielLeens for the CI triage on `ff321c7b6` and for confirming the `f77cdfa67` run came back green across the matrix — the failure-by-failure breakdown makes it easy to separate infra noise from PR-related regressions. One clarification before I can treat this as ready from my side: the comment says "no open code-level blockers", but I don't see responses in this thread to the findings from my previous review, so I can't tell whether they were addressed in the intervening commits or are still pending. Could you (or the author) reply to each of these, either pointing to where it was fixed or explaining why it's not applicable? - **PR11503-F1 (HIGH, Bug)** – `IncrementalSourceReader`: the legacy checkpoint restore path hardcodes the tablePath, so the restored `CatalogTable`'s `TableId` can't match the real source table. This is the one I'd consider a hard blocker; is the real table path now recovered from the checkpoint? - **PR11503-F5 (Functional)** – same class: the restore gate widened from `checkpointDataType != null` to any non-empty `checkpointTables`, meaning every restored CDC job goes down the restore/event path even with no DDL history. Is that intentional, and is there test coverage for the no-DDL restore case? - **PR11503-F2 (Compatibility)** – `RestoreTableSchemaEvent` flows through the generic `SchemaChangeEvent` SPI; connectors/transforms not updated in this PR will hit unknown-event paths during failover recovery. What's the fallback behavior for them? - **PR11503-F3 (Robustness)** – `AlterTableSchemaEventHandler`: a null `changeAfter` on `RestoreTableSchemaEvent` silently falls through to stale-schema behavior; this should fail loudly. - **PR11503-F6 (Logic)** – `TableSchemaChangeEventDispatcher`: the restore shortcut replaces the current schema without verifying the event targets the same table. - **PR11503-F4 (Robustness)** – `AbstractDebeziumDeserializationSchema`: `restoreCheckpointHistoryTableChanges` does a non-atomic `clear()`+`putAll()` on a shared, non-concurrent map. - **PR11503-F7 (Docs)** – Javadoc for the new public SPI method `restoreCheckpointHistoryTableChanges` in `DebeziumDeserializationSchema`. - **PR11503-F8 (Security)** – `IncrementalSourceReader` logs the full `CatalogTable` list (schema metadata + options map) at INFO on every restore; please drop to DEBUG and/or log only table identifiers. Once the findings above have a status (fixed / won't-fix with rationale), I'm happy to do the final pass at the current head. <!-- streview-comment:895 --> -- 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]
