SEZ9 commented on PR #11162: URL: https://github.com/apache/seatunnel/pull/11162#issuecomment-6514037704
Thanks for the work on this one. Following up on the current head `e9c52313cfba2a64b713f743be95e80a00794072`: **CI gate** The [Build](https://github.com/apache/seatunnel/runs/121486382851) check is still `ACTION_REQUIRED` because no fork Build run was found for this SHA, while [Signal pull request update](https://github.com/apache/seatunnel/actions/runs/40166998382/job/121480690360) is `SUCCESS`. This is an activation gap, not a test failure. Please open the Actions tab on your fork, enable workflows if they are disabled, and manually run the existing Build workflow for this branch. If a run for this exact head already exists, drop the link here and we'll reconcile it. No empty commit, dependency bump or base sync is needed for this — none of those can supply the missing run. **Open items from the [previous review](https://github.com/apache/seatunnel/pull/11162#pullrequestreview-5642904306)** These remain unaddressed on the unchanged head, so the requested-changes state still stands. Concretely: 1. **EVOLVE default vs. deprecated no-op writers (F1, HIGH)** — With `schema-changes.enabled = true`, a sink whose writer still inherits the deprecated no-op `applySchemaChange` now fails where it previously ran. Either keep the legacy behavior as the default for such writers, or drop the "backward compatibility is preserved" wording and document this as the breaking change it is in `docs/en/introduction/concepts/incompatible-changes.md`. Please state which direction you're taking. 2. **Fail-fast restart loop (F2, HIGH)** — Deterministic failures under `strict`/`ignore` (and `evolve` on unsupported events) will re-trigger on every checkpoint restore. Please describe how the job terminates rather than looping, or make the failure terminal, instead of leaving that to operators in `docs/en/introduction/configuration/schema-evolution.md`. 3. **Override detection (F3)** — Please confirm the reflection check walks the full class hierarchy, is cached, and behaves correctly for wrapped/decorated writers. A short note or test pointer is fine. 4. **`behavior` × `include`/`exclude` (F4, F6)** — Both `MySQL-CDC.md` and `schema-evolution.md` need a sentence on whether filtering is applied before the behavior policy (i.e. does a filtered-out event still fail under `strict`?). 5. **`ignore` ≡ `strict` for supported events (F5)** — Please say this explicitly in the docs so users don't assume `ignore` is a lenient mode. 6. **Deprecation window (F7)** — Add a target removal version for the deprecated `applySchemaChange`, and align the incompatible-changes entry with the PR description. 7. **`behavior` set while `enabled = false` (F8)** — Add a config-time validation error or at least a warning rather than silently ignoring the option. Discussion replies are welcome, but I'll only mark items resolved against committed changes. Once the fork Build run is linked and a new head lands with the above, I'll re-review. <!-- streview-comment:2140 --> -- 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]
