davidzollo commented on PR #11503:
URL: https://github.com/apache/seatunnel/pull/11503#issuecomment-5425538451
Pushed the fix for the blocking NPE. New head: `1e5493c9f8`.
**What changed (`24d4d2e92adf..1e5493c9f8`):**
-
`IncrementalSourceReaderTest.restoreCheckpointStateRestoresTablesAndHistoryFromNewCheckpointFormat`
now stubs `getTablePath()` on the mocked `CatalogTable`
(`TablePath.of("catalog", "database", "new_table")`) — exactly the fix
@DanielLeens suggested. The unstubbed mock NPEd in `toCheckpointTablePaths`
once the module compiled again. No production-code change: every real
`CatalogTable` carries a non-null `TablePath`, so no null-guard was added.
- `RestoreTableSchemaEvent` class Javadoc now states the consumer contract
explicitly: `SCHEMA_CHANGE_RESTORE` means runtime schema refresh only — never
physical DDL re-application. This lands the actionable part of PR11503-F2's
"make the fallback contract explicit" ask.
**Disposition of PR11503-F1..F8** for @SEZ9's next full pass (F-numbers from
your 2026-08-25 summary, matching Issues 1-8 of the 2026-08-22 review):
| Finding | Status at `1e5493c9f8` |
|---|---|
| F1 legacy `default.default` identity (HIGH) | **Fixed** in the
`b99b1a1b9`/`6e4c00f6b`/`ff4463922` round: identity is derived from
`IncrementalSplit#getTableIds()` (including the `MultipleRowType` per-table
case), and an unrecoverable identity now skips restore with a warning instead
of fabricating one. Three unit tests cover the three branches. Independently
re-verified line-by-line in DanielLeens' 2026-08-23 review. |
| F2 unknown-event paths | **Contract now documented** on the event class
(this push). The in-tree audit in the 2026-08-23 review found the only
untouched `SupportSchemaEvolution` sink, Console, delegates entirely to the
shared `DataTypeChangeEventDispatcher`, which already special-cases restore
centrally — safe with zero code change. A wider third-party-connector audit
remains a follow-up. |
| F3 null `changeAfter` fail-fast | **Fixed** in `b99b1a1b9`:
`Objects.requireNonNull` in the constructor plus `getRestoredTable()` throwing
`IllegalStateException`; all three dispatch sites use it, so a broken event now
fails fast and identically everywhere. Covered by
`restoreEventRejectsNullCatalogTable` and
`dispatchersFailFastWhenRestoreEventLosesChangeAfter`. |
| F4 non-atomic `clear()+putAll()` | **Race/corruption closed** in
`b99b1a1b9` via `synchronized (tableChangesStructMap)` around restore, read,
and the deserialize-path writes. The wholesale-replace (vs merge) semantics are
retained deliberately: on restore, the checkpoint history is the source of
truth for that split, and merging could keep stale live entries the checkpoint
intentionally superseded. The residual merge-semantics question stays a
follow-up (downgraded to Low in the 2026-08-23 re-review). |
| F5 restore gate widened | **Rationale, no change:** event emission is
gated per table by an equality check in
`SeaTunnelRowDebeziumDeserializeSchema.restoreCheckpointProducedType`
(`!latestTable.getSeaTunnelRowType().equals(restoreTable.getSeaTunnelRowType())`).
The reader-side gate only controls whether restore processing runs — not
whether an event is emitted. A failover with no DDL delta emits no restore
event. |
| F6 dispatch identity check | **Rationale, no change in this PR:** the
dispatchers' `reset(...)` context is a bare `SeaTunnelRowType` / `TableSchema`
with no table identity to compare against
(`DataTypeChangeEventDispatcher.reset(SeaTunnelRowType)`,
`TableSchemaChangeEventDispatcher.reset(TableSchema)`), and per-table instance
routing is the same trust boundary every existing `AlterTableEvent` subtype
already relies on. Kept as a defense-in-depth follow-up (it would need an
SPI-level identity in the reset context) rather than expanded inside this fix
PR. |
| F7 SPI Javadoc | **Fixed** in the `b99b1a1b9` round:
`restoreCheckpointHistoryTableChanges` documents invocation timing, the
`byte[]` encoding (serialized Debezium `TableChanges` structs), and the replace
semantics of the base implementation. |
| F8 INFO log dump | **Fixed** in the `b99b1a1b9` round: restore logging now
emits table count plus table paths only (`toCheckpointTablePaths`), for both
the primary and legacy paths. |
**CI:** the push triggered a fresh Build on the fork head `1e5493c9f8`. The
previous run's only failures besides the NPE were the known unrelated
Couchbase/DynamoDB container flakes; I am tracking the new run and will report
the result here. If those flakes recur, I will retry at the smallest
granularity available (failed jobs only).
--
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]
