DanielLeens commented on PR #11503:
URL: https://github.com/apache/seatunnel/pull/11503#issuecomment-5601242152
Checked the stored body via the API rather than the web renderer, same as
the last time this came up in one of our threads — it is not actually
truncated, the full F1-F8 table and the closing "Merge Recommendation" section
are both there. So this looks like the same rendering artifact, just hitting
your side this time. Re-pasting the table in full here per your ask, so there's
nothing to infer:
| Finding | Status now | Evidence |
|---|---|---|
| F1 (High) — legacy path hardcodes `default.default` | **Fixed.** |
`IncrementalSourceReader.restoreLegacyCheckpointTables()`/`toLegacyCheckpointTable()`
(lines ~296-336) now derive `TablePath.of(tableId)` from
`incrementalSplit.getTableIds()` for the single-table case and from
`MultipleRowType.getTableIds()` for the multi-table case; falls back to
`Collections.emptyList()` (skip restore, log a WARN) only when identity
genuinely can't be recovered (`tableIds.size() != 1`). Covered by
`IncrementalSourceReaderTest.restoreCheckpointStateRestoresLegacyCheckpointDataType`
(asserts `"catalog.database.customers"`, not `"default.default"`),
`...RestoresLegacyMultipleCheckpointTables`, and
`...SkipsLegacyCheckpointDataTypeWithoutRecoverableTableIdentity`. |
| F2 (Medium) — unknown-event fallback for untouched connectors |
**Substantially addressed, worth one more line.** | See "Key findings" in my
last review — the shared-handler centralization plus `SinkWriter`'s no-op
default means untouched consumers cannot crash or misfire DDL. What's not
present is an explicit test/doc enumerating "these connectors get it via the
shared dispatcher, these get it via an explicit patch, everything else safely
no-ops" — kept open as a Low, non-blocking recommended fix. |
| F3 (Medium) — null `changeAfter` should fail loudly | **Fixed.** |
`RestoreTableSchemaEvent.getRestoredTable()` now throws
`IllegalStateException("RestoreTableSchemaEvent requires changeAfter to be
present.")` instead of returning null and letting callers fall through; the
constructor also does `Objects.requireNonNull(restoredTable, ...)` so the only
way to get a null `changeAfter` post-construction is an explicit
`setChangeAfter(null)` downstream, now caught at the next `getRestoredTable()`
call. |
| F4 (Medium) — non-atomic `clear()+putAll()` on shared map | **Fixed.** |
`AbstractDebeziumDeserializationSchema.java:62-74`: `getHistoryTableChanges()`,
`restoreCheckpointHistoryTableChanges()`, and the live `deserialize()` mutation
path all now `synchronized (tableChangesStructMap)`. Still fully replaces (not
merges) the map on restore, but since it's inside the same lock as every other
accessor, the specific race you described (a `deserialize()` call landing
between `clear()` and `putAll()`, e.g. via `addSplitsBack()` while the pipeline
is live per #11677) is closed. |
| F5 (Medium) — gate widened to any non-empty `checkpointTables`, even
no-DDL restores | **Fixed in effect, and now the intentional design — verified
by test.** | The `IncrementalSourceReader`-level gate is indeed wider (any
non-empty `checkpointTables` triggers `restoreCheckpointProducedType`), because
`snapshotCheckpointDataType()` populates `checkpointTables` on every
checkpoint, not just ones following a DDL. But the actual downstream signal is
gated on an actual row-type inequality inside
`SeaTunnelRowDebeziumDeserializeSchema.restoreCheckpointProducedType()`.
`SeaTunnelRowDebeziumDeserializeSchemaRestoreTest.doesNotEmitRestoreEventWhenSchemaDidNotChange`
proves this directly: restoring with an identical schema never calls
`collector.collect(any(RestoreTableSchemaEvent.class))`. So a no-DDL failover
restore rebuilds internal converters (harmless) but never injects a
schema-change event into a pipeline that's never seen one. |
| F6 (Medium) — restore short-circuit doesn't check table identity | **Not
fixed at the dispatcher level, but not exploitable on any currently-wired call
path.** | This is the one I'm downgrading, and here's the call-graph evidence:
`TableSchemaChangeEventDispatcher.apply()`,
`DataTypeChangeEventDispatcher.apply()`, `AlterTableSchemaEventHandler.apply()`
still unconditionally return the restored table's schema/row-type for a
`RestoreTableSchemaEvent`, without comparing it against the identity the
handler/dispatcher instance was scoped for. However, both real call paths that
route `SchemaChangeEvent`s to per-table handler instances pre-route by
identity: `MultiTableSinkWriter.applySchemaChange()`
(`MultiTableSinkWriter.java:362-397`) routes by
`event.tablePath().getFullName()` to the matching per-table `SinkWriter` before
that writer's own dispatcher ever sees the event, and
`AbstractMultiCatalogTransform.mapSchemaChangeEvent()`
(`AbstractMultiCatalogTransform.java:175-179`) does
the identical routing by `event.tablePath()` to the matching per-table
`targetTransform`. A single-table (non-multi-table) sink/transform only ever
has one possible table identity to begin with. I could not find a call path
where a dispatcher instance scoped to table A receives a
`RestoreTableSchemaEvent` for table B — so it's real, cheap, defense-in-depth
hardening worth adding (a two-line identity check + log-and-skip on mismatch
protects against a *future* caller that doesn't pre-route), but not a live
correctness bug on the code as it exists today. That's why it's Low rather than
the original Medium, and non-blocking. |
| F7 (Medium) — missing Javadoc on new SPI method | **Fixed.** |
`DebeziumDeserializationSchema.java:47-56` now has a full Javadoc block on
`restoreCheckpointHistoryTableChanges` covering invocation timing and the
"replace stale history" contract. `RestoreTableSchemaEvent` itself also gained
class-level Javadoc documenting the `SCHEMA_CHANGE_RESTORE` consumer contract. |
| F8 (Medium) — full `CatalogTable` list logged at INFO | **Fixed.** |
`IncrementalSourceReader.restoreCheckpointState()` now logs via
`toCheckpointTablePaths(checkpointTables)` (table-path strings only), not the
full `CatalogTable` objects (which would include the options map). |
Net, unchanged from my last review: 6 of 8 fixed-and-verified (F1 and F5
with dedicated new unit tests proving the exact scenario), F2 substantially
mitigated (doc gap only, Low), F6 real-but-unreachable on any wired call path
today (downgraded Medium to Low with the call-graph trace above). No
High/Medium source-level issue remains open. Conclusion stands: **Ready to
merge**, blocked only on a write-permission maintainer's sign-off, not on
anything code-side.
Happy to do a fresh independent check on your end once you've had a chance
to compare — nothing new from me beyond this repost.
--
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]
