DanielLeens commented on PR #11503:
URL: https://github.com/apache/seatunnel/pull/11503#issuecomment-5611996446
@nzw921rx @davidzollo I re-traced the P1 finding independently against the
current head (`f77cdfa677d4`) rather than taking the description on faith,
since it's a silent-data-staleness claim in exactly the restore path this PR is
about.
It checks out end to end:
- `TableEvent.java:33` — `private long createdTime =
System.currentTimeMillis();` is a field initializer, so every
`RestoreTableSchemaEvent` gets its timestamp at construction time with
millisecond resolution.
- `IncrementalSourceReader.java`'s new `emitPendingRestoreSchemaEvents(...)`
constructs one `RestoreTableSchemaEvent` per changed table inside a plain `for`
loop and calls `collector.collect(new RestoreTableSchemaEvent(restoreTable))`
for each — no explicit spacing between constructions.
- `SchemaOperator.java:375-386` (`applyNextPendingSchemaChange`) reads
`eventTime = event.getCreatedTime()` and skips as outdated whenever
`lastProcessedEventTime != null && eventTime <= lastProcessedEventTime`, with
**no `RestoreTableSchemaEvent`/`SCHEMA_CHANGE_RESTORE` exemption** anywhere in
that check.
So the failure mode is real: two tables restored in the same batch that
happen to construct their events in the same millisecond → the second one is
silently dropped with only a `log.warn`, and per this PR's own new Javadoc on
`RestoreTableSchemaEvent` ("only refreshes runtime schema state... row types,
converters, serializers"), that table's sink-side runtime schema simply never
gets refreshed. That's a quiet correctness regression sitting inside the exact
recovery path this PR exists to fix, not an unrelated pre-existing corner.
I'd push back on treating this as deferrable to a follow-up PR, for two
reasons: (1) "low probability" undersells it — this is a plain wall-clock tight
loop, and per-table `log.info` calls before each `collect()` don't guarantee
>1ms spacing, especially with buffered/async appenders; (2) even if rare, the
failure is silent (a `log.warn` next to a stream of other info logs) rather
than loud, which is the worse failure mode for a schema-recovery path.
The fix nzw921rx proposed — exempting the idempotent restore event from the
outdated-event check — is small and localized (one extra `instanceof
RestoreTableSchemaEvent` condition alongside the one already added at
`SchemaOperator.java:170-171` for the `supportedTypes` check), and it's
consistent with this PR's own contract that restore events are idempotent
runtime-only refreshes with no DDL side effect, so replaying/reordering them
relative to `lastProcessedEventTime` isn't actually unsafe to skip. Given the
fix is this contained, I'd rather see it land in this PR than tracked
separately — happy to be told I'm missing something that makes the follow-up-PR
route necessary, but from the source alone I don't see why this needs to be
split out. The merge-conflict flag nzw921rx raised separately also still needs
resolving before this can go in 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]
