davidzollo commented on PR #10951:
URL: https://github.com/apache/seatunnel/pull/10951#issuecomment-4563339117
Re-posting this under Daniel because the previous review submission from my
environment was attributed incorrectly. Please treat this Daniel-authored
comment as the authoritative maintainer review for the current head.
Thanks for working on this. I re-reviewed the latest head from scratch and
traced the current Flink 1.13 schema-evolution path again, including
`SourceExecuteProcessor -> FlinkSourceReader keep-alive -> SchemaOperator
checkpoint/fallback -> BroadcastSchemaSinkOperator -> FlinkSinkWriter`.
The direct-commit blocker from the previous head is gone, but I still found
one blocking correctness gap in the current fallback path.
Runtime path I checked:
```text
DDL detection
-> SchemaOperator.processElement(...)
-> schema change enters pendingQueue and starts fallback timer
Normal safe path
-> notifyCheckpointComplete(...)
-> firstSeenCheckpointId is recorded first
-> after the extra completed checkpoint round,
applyNextPendingSchemaChange(...)
Current fallback path
-> handleFallbackTimerOnTaskThread(...)
-> when no checkpoint completes in time, it calls
applyNextPendingSchemaChange(...) directly
-> this happens even if the post-DDL checkpoint safety round never
completed
```
Problem 1: the fallback timer now bypasses the checkpoint-completion safety
fence that the operator itself relies on to avoid XA/MDL conflicts
- Location:
-
`seatunnel-translation/seatunnel-translation-flink/seatunnel-translation-flink-common/src/main/java/org/apache/seatunnel/translation/flink/schema/SchemaOperator.java:225-238`
-
`seatunnel-translation/seatunnel-translation-flink/seatunnel-translation-flink-common/src/main/java/org/apache/seatunnel/translation/flink/schema/SchemaOperator.java:296-321`
-
`seatunnel-translation/seatunnel-translation-flink/seatunnel-translation-flink-common/src/test/java/org/apache/seatunnel/translation/flink/schema/SchemaOperatorTest.java:179-201`
- Severity: High
- Raised before by others: No
- Why this is a real bug:
The operator comment explicitly says the DDL must wait for an extra
completed checkpoint round so the earlier XA commit can finish before ALTER
TABLE runs. But the new fallback path skips that fence completely: if no
checkpoint completes after the schema event,
`handleFallbackTimerOnTaskThread(...)` jumps straight into
`applyNextPendingSchemaChange(...)`. The new unit test even locks in that
behavior by asserting the fallback succeeds without any checkpoint completion.
- Risk:
This reopens the exact safety boundary the operator comment is trying to
protect. Under the stalled-checkpoint scenario, the DDL can now run while the
prior transactional commit / MDL-sensitive work may still be in flight.
- Better fix:
Keep the Flink 1.13 keep-alive improvement, but do not let the fallback
timer bypass the checkpoint-based safety fence. If a true fallback is still
needed, it should preserve the same post-checkpoint ordering guarantee instead
of applying the DDL immediately.
### Conclusion: fix required before merge
Blocking items:
1. Problem 1
Non-blocking suggestions:
- The keep-alive direction itself is reasonable and worth preserving once
the fallback ordering is made safe.
--
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]
