SEZ9 commented on PR #11162: URL: https://github.com/apache/seatunnel/pull/11162#issuecomment-6538865736
Following up on the previous review — the head is still `e9c52313cfba2a64b713f743be95e80a00794072`, so the earlier points remain open. Nothing here is new scope; this is just a consolidated list of what I still need to see committed before the next full pass: **HIGH** - **F1 – EVOLVE default vs. deprecated no-op writers.** Existing `schema-changes.enabled = true` jobs whose sink writer still inherits the deprecated no-op will now fail instead of running. Either keep a compatibility default for those writers or drop the "backward compatibility is preserved" wording and document the behavior change plainly in `docs/en/introduction/concepts/incompatible-changes.md`. Please state which option you're taking. - **F2 – Deterministic fail-fast → restart loop.** With `strict`/`ignore` (and `evolve` on unsupported events) the job will hit the same event after every checkpoint restore. I'd like to see the engine fail terminally (or at least a documented, bounded behavior) rather than `docs/en/introduction/configuration/schema-evolution.md` telling operators to manage the loop. **MEDIUM** - **F3 – Override detection.** Please confirm (ideally with a test) that the "explicitly overrides the deprecated method" check walks the class hierarchy, handles wrapped/decorated writers, and caches the result. - **F4 / F6 – `behavior` × `include`/`exclude`.** Define the interaction once (is filtering applied before the policy? does an excluded event ever trigger `strict` failure?) and reference it from both `docs/en/connectors/source/MySQL-CDC.md` and the schema-evolution doc. - **F5 – `ignore` ≡ `strict` for supported events.** Say this explicitly in the docs so users don't pick `ignore` expecting a softer mode. **LOW** - **F7 – Removal timeline** for the deprecated `applySchemaChange` path, and reconcile the incompatible-changes entry with the PR description. - **F8 – `schema-changes.behavior` with `enabled = false`.** A config-time warning or validation error is enough. On CI: `Signal pull request update` (run 40166998382 / job 121480690360) is `SUCCESS`, but `Build` (121486382851) is `ACTION_REQUIRED` — please trigger/link the fork Build for this exact head so we have a real result to review against. Once the fixes above land in a new commit I'll do the next complete review. Thanks! <!-- streview-comment:2168 --> -- 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]
