JeremyXin commented on PR #11932:
URL: https://github.com/apache/seatunnel/pull/11932#issuecomment-5475406548
Reviewed the latest head `4414b55`.
The core approach is sound: gating collector-side schema restore behind
`getSchemaChangeResolver() != null` ensures the source and deserializer sides
restore consistently, and the one-shot `restoredCheckpointTables` pattern keeps
the hot path clean. The `JdbcSink` refactor (unified
`createWriter/restoreWriter`, `getWriterStateSerializer`, schema carried in
state) is correct and the E2E test covers the actual regression.
### Merge Conclusion: Comment
Two points worth addressing before merge:
1. **Impact scope for `Collector.restoreSchema`**: `IncrementalSourceReader`
is fixed, but
are there other CDC source readers (e.g. Postgres CDC, MongoDB CDC) that
use a different
base class and face the same issue? A brief note on which connectors are
affected / not
affected would help confirm coverage.
2. **`restoredCheckpointTables` read+clear is non-atomic**: the current
pattern reads the
field into a local variable and then sets it to null in two separate
steps. If `pollNext`
is guaranteed to run on a single thread, a quick comment confirming that
assumption would
be helpful.
--
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]
