SEZ9 commented on PR #11162: URL: https://github.com/apache/seatunnel/pull/11162#issuecomment-6486786012
Thanks for the contribution. Two things are still outstanding on this PR, neither of which has moved since the last review. **CI gate.** On head `e9c52313cfba2a64b713f743be95e80a00794072` the [Build](https://github.com/apache/seatunnel/runs/121486382851) check is `ACTION_REQUIRED` because no fork Build run was found for that SHA. This is not a test failure, and I'm not asking for an empty commit, a dependency bump, or a base sync. Please open the Actions tab on your fork, enable workflows if they are disabled, and manually trigger the existing Build workflow for this branch. If a matching run already exists, link it here so we can reconcile the aggregate status. I won't pass CI by inference. **Source/documentation points from the [previous review](https://github.com/apache/seatunnel/pull/11162#pullrequestreview-5642904306)** — these are unchanged and still need a response or a code/doc update: - **HIGH – EVOLVE default vs. compatibility claim.** With `schema-changes.enabled = true`, a sink writer that still inherits the deprecated no-op will now fail where it previously ran. Please either change the default so existing jobs keep running, or drop the "backward compatibility is preserved" wording and document this as a real behavior change in `docs/en/introduction/concepts/incompatible-changes.md`. - **HIGH – restart loop on deterministic failure.** `strict`/`ignore` (and `evolve` on an unsupported event) fail deterministically, so checkpoint-restore will replay the same event and fail again indefinitely. Please describe how the engine terminates the job rather than leaving operators to detect the loop, and reflect that in `docs/en/introduction/configuration/schema-evolution.md`. - **MEDIUM – "explicitly overrides the deprecated method" detection.** Please confirm the reflection walk covers the full class hierarchy (inherited overrides), is cached, and works for wrapped/decorated writers. A pointer to the relevant code or a unit test would be enough. - **MEDIUM – `behavior` × `include`/`exclude`.** Both `docs/en/connectors/source/MySQL-CDC.md` and the new "Schema change behavior" section leave undefined what happens to an event that is filtered out by `include`/`exclude`: is it dropped before the behavior policy applies, or does `strict` still fail on it? Please state this explicitly in both places. - **MEDIUM – `ignore` ≈ `strict`.** For every evolution-supported event type the two policies behave the same; please say so directly in the docs so users don't pick `ignore` expecting it to skip events. - **LOW – deprecation window.** Please add a target removal version for the deprecated `applySchemaChange` path, and align the incompatible-changes entry with the PR description. - **LOW – `behavior` set while `enabled = false`.** This is currently silently ignored; a config-time validation error or at least a warning log would help. Once the Build workflow is running on the current head and the items above have been addressed (or you've explained why a given one shouldn't change), I'll take another full look. <!-- streview-comment:2114 --> -- 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]
