DanielLeens commented on PR #11060: URL: https://github.com/apache/seatunnel/pull/11060#issuecomment-5495783023
Thanks for pushing this through, @davidzollo — I checked commit `50ddffe` on `codex/fix-pr-11060-oceanbase-mode` directly against the diff, not just the description. It holds up as a fix for both items from my last round: - **[P1] `compatible_mode`**: `OceanBaseIncrementalSourceFactory` now overrides `optionRule()` to declare `compatible_mode` as optional with a regex condition restricting it to `mysql`, and `mysqlCompatibleConfig()` defaults it to `mysql` when omitted and throws `IllegalArgumentException` for anything else (Oracle mode included) before the source is constructed. That closes all three gaps I flagged: the doc examples validate again (key is now declared), the E2E's explicit `compatible_mode = "mysql"` passes static validation instead of only working because the job skips validation, and Oracle mode is now rejected instead of silently routing catalog discovery to `OceanBaseOracleCatalog` while the runtime stays MySQL-binlog. - **[P2] duplicated `restoreSource`**: the shared table-building logic is extracted into a new protected `buildCatalogTables(...)` on `MySqlIncrementalSourceFactory`, and the OceanBase factory now just wraps the config through `mysqlCompatibleConfig()` before delegating to it. That's the same shared-extraction shape I suggested, so the two factories can't silently drift on this logic again. - New tests (`OceanBaseIncrementalSourceFactoryTest`) cover default-to-mysql, explicit-mysql-accepted, and oracle-rejected — matching the three scenarios from my finding. I haven't done a full line-by-line re-review of this commit (e.g. haven't verified it compiles cleanly against current `dev` or re-checked `optionRule()`/`Conditions` API usage in detail), but on inspection it plausibly resolves both the P1 blocker and the P2 follow-up. One mechanical snag: this PR's head branch (`daniel_issue_11049_oceanbase_cdc`) lives on `DanielLeens/seatunnel`, and that repo has `Allow edits from maintainers` off (`maintainer_can_modify: false`), so I can't push or apply a commit onto it myself — I only have read access here as a reviewer, not push access to that fork. A couple of ways to get `50ddffe` onto this PR: 1. Whoever controls the `DanielLeens/seatunnel` fork applies/cherry-picks `50ddffe` onto `daniel_issue_11049_oceanbase_cdc` and pushes it — that keeps this PR's history and review thread intact. 2. Alternatively, since you're the PR author, flipping "Allow edits from maintainers" on for this PR (if GitHub still offers that toggle given the cross-account setup) would let a maintainer push directly. 3. If neither is workable, opening a fresh PR from `davidzollo:codex/fix-pr-11060-oceanbase-mode` against `apache/seatunnel:dev` and closing this one with a pointer would also work, though it'd lose this thread's review history. Once the commit is actually on this PR's head, I'll do a full re-review from there. -- 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]
