DanielLeens commented on PR #11503: URL: https://github.com/apache/seatunnel/pull/11503#issuecomment-5617115021
@SEZ9 Checked the head first: `f77cdfa677d4` is still the current head (same commit nzw921rx and I were both looking at), nothing has landed since your 2026-09-09T02:59:14Z comment or my 2026-09-09T11:40:15Z follow-up, so this is the same source I already walked through item-by-item, not a case of new code needing a fresh pass. To save you re-reading the whole table, here's the F1-F8 status again with the evidence, since your list matches your original Aug-22 items 1:1: - **F1 (HIGH, `default.default` hardcode)** — Fixed. `IncrementalSourceReader.restoreLegacyCheckpointTables()`/`toLegacyCheckpointTable()` derive `TablePath.of(tableId)` from `incrementalSplit.getTableIds()` (single-table) or `MultipleRowType.getTableIds()` (multi-table), and skip restore with a WARN log only when identity genuinely can't be recovered. Covered by `IncrementalSourceReaderTest.restoreCheckpointStateRestoresLegacyCheckpointDataType` (asserts the real `"catalog.database.customers"` path, not `"default.default"`) plus the multi-table and ambiguous-identity variants. - **F2 (MEDIUM, unknown-event fallback)** — Substantially mitigated, kept open as Low. The shared `TableSchemaChangeEventDispatcher`/`AlterTableSchemaEventHandler` patch plus `SinkWriter`'s no-op `applySchemaChange` default mean an untouched connector/transform can't crash or misfire DDL on a `RestoreTableSchemaEvent` — verified structurally by tracing every DDL-issuing sink's guard. What's still missing is an explicit doc/test enumerating which connectors get this via the shared dispatcher vs. an explicit guard vs. the no-op default — that's Issue 2 in my open, non-blocking list. - **F3 (MEDIUM, null changeAfter)** — Fixed. `RestoreTableSchemaEvent.getRestoredTable()` throws `IllegalStateException` instead of silently returning null, and the constructor does `Objects.requireNonNull(restoredTable, ...)`. - **F4 (MEDIUM, non-atomic clear+putAll)** — Fixed. `AbstractDebeziumDeserializationSchema` now synchronizes `getHistoryTableChanges()`, `restoreCheckpointHistoryTableChanges()`, and the live `deserialize()` mutation path on the same monitor (`tableChangesStructMap`), closing the window you flagged. - **F5 (MEDIUM, gate widened to any non-empty checkpointTables)** — Fixed in effect and test-verified as intentional. The reader-level gate is wider by design, but the actual event that reaches transforms/sinks is gated on an actual row-type inequality in `SeaTunnelRowDebeziumDeserializeSchema.restoreCheckpointProducedType()`; `SeaTunnelRowDebeziumDeserializeSchemaRestoreTest.doesNotEmitRestoreEventWhenSchemaDidNotChange` proves a no-DDL restore never emits a `RestoreTableSchemaEvent`. - **F6 (MEDIUM, restore short-circuit skips identity check)** — Real at the code level, downgraded to Low: not reachable on any currently-wired call path. `MultiTableSinkWriter.applySchemaChange()` and `AbstractMultiCatalogTransform.mapSchemaChangeEvent()` both route by `event.tablePath()` to the correct per-table writer/transform before the dispatcher ever sees the event, and single-table writers only have one possible identity. Still worth a two-line defensive check for future callers — that's Issue 1 in my open list — but not a live bug today. - **F7 (MEDIUM, missing Javadoc)** — Fixed. `DebeziumDeserializationSchema.restoreCheckpointHistoryTableChanges` and `RestoreTableSchemaEvent`'s class doc both now carry the contract explanation. - **F8 (MEDIUM, full CatalogTable logged at INFO)** — Fixed. `IncrementalSourceReader.restoreCheckpointState()` logs table-path strings only (`toCheckpointTablePaths(...)`) at INFO, not the full `CatalogTable` with its options map. So from my side: 6 of 8 are fixed-and-tested, F2 and F6 are downgraded to Low non-blocking with the reasoning above, and I don't see a remaining High/Medium source-level blocker among these eight on `f77cdfa677d4`. If you're seeing something different in the actual code (not just the summary table), a `path:line` pointer on the specific item would let me recheck against the exact code you mean. Agreed on the merge conflicts nzw921rx flagged — those need to be resolved against `dev` regardless of the F1-F8 status, and I'd suggest doing that first since it'll be needed before another CI run means anything either way. -- 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]
