SEZ9 commented on PR #11503: URL: https://github.com/apache/seatunnel/pull/11503#issuecomment-5612186438
Thanks @nzw921rx for the approval, and +1 on resolving the merge conflicts before we go further. From my side, the points from my earlier review still look open against the current head. Summarizing what I'm asking for on each: - **F1 (HIGH, Bug)** — the legacy checkpoint restore path in `IncrementalSourceReader` hardcodes the table path `default.default`, so the restored `CatalogTable`'s `TableId` cannot match the real source table. Please derive the real table path/`TableId` from the checkpoint data (or reject the legacy state loudly) and add a unit test that restores a non-default table. - **F2 (MEDIUM, Compatibility)** — `RestoreTableSchemaEvent` flows through the generic `SchemaChangeEvent` SPI, so connectors/transforms not updated in this PR may hit unknown-event paths during failover recovery. Please either update the affected handlers here, or document the compatibility story and make the unknown-event case a warning rather than a failure. - **F3 (MEDIUM, Robustness)** — a null `changeAfter` on `RestoreTableSchemaEvent` silently falls through to stale-schema behavior. Please fail fast instead. - **F4 (MEDIUM, Robustness)** — `restoreCheckpointHistoryTableChanges` does a non-atomic `clear()` + `putAll()` on a shared, non-concurrent map. Please build the new map and swap the reference atomically, or guard it with the same lock the readers use. - **F5 (MEDIUM, Functional)** — the restore gate was 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 confirm this is intentional and explain why in the PR description; otherwise narrow the condition back. - **F6 (MEDIUM, Logic)** — the restore shortcut in the schema dispatcher replaces the current schema without verifying the event targets the same table. Please add a table-identity check before replacing. - **F7 (MEDIUM, Docs)** — the new public SPI method `restoreCheckpointHistoryTableChanges` needs Javadoc (contract, when it is invoked, thread-safety expectations). - **F8 (MEDIUM, Security)** — the full `CatalogTable` list (schema metadata + options map) is logged at INFO on every restore. Please log only table ids at INFO and move the full dump to DEBUG, or redact the options. If any of these were already addressed in a push I missed, a quick pointer per item is enough and I'll re-check. Once the conflicts are resolved and the above are covered, I'm happy to take another pass. <!-- streview-comment:946 --> -- 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]
