DanielLeens commented on PR #11029: URL: https://github.com/apache/seatunnel/pull/11029#issuecomment-5390001346
Thanks for the deep re-review, @SEZ9 — this is on the same head (`075001b0b5a9`) I approved on Aug 14, so I went back and checked the two points that matter most before saying anything. **Issue 1 (startup.mode narrowing) — I agree, this reopens a real blocker.** I diffed this file against the merge-base: before this PR, `OpengaussIncrementalSourceFactory` wired the shared `PostgresSourceOptions.STARTUP_MODE`, whose choice set is `INITIAL, SNAPSHOT_ONLY, COMMITTED_OFFSET, EARLIEST, LATEST`. The new `OpengaussSourceOptions.STARTUP_MODE` narrows that to `INITIAL, EARLIEST, LATEST` only. `SNAPSHOT_ONLY` is driven by the shared incremental framework (`SnapshotOnlySplitAssigner`, gated in `IncrementalSource.createEnumerator`/`getBoundedness`) rather than by anything Postgres-dialect-specific, so it isn't in the same category as `COMMITTED_OFFSET` (which genuinely does read Postgres-only `pg_replication_slots` columns). That means any existing `Opengauss-CDC` job with `startup.mode = snapshot-only` will now fail `OptionRule` validation at submit with no replacement. That's a straightforward backward-compat break and I missed it in my approval — good catch. **Issue 3 (getStartupModeOption() as a constructor-time virtual call) — confirmed, and I agree it's the same footgun class.** `IncrementalSource`'s constructor calls `getStartupConfig(readonlyConfig)`, which calls `config.get(getStartupModeOption())`, before the subclass constructor body runs. `OpengaussIncrementalSource.getStartupModeOption()` only happens to be safe today because it returns a stateless static constant — but that's exactly the same shape of bug as the `require-replica-identity-full` init-order issue this PR fixes elsewhere. Agreed this is worth hardening (constructor arg or a `requireNonNull`/Javadoc guard), non-blocking given it's currently harmless. I haven't re-traced issues 2/4/5/6/7/8 line-by-line in this pass, but they read as consistent with the same compatibility/documentation theme and I don't have reason to push back on any of them. Given Issue 1 is a genuine blocker, I'm treating my Aug 14 approval as superseded — this needs another pass once the startup-mode compatibility gap is closed (either add `SNAPSHOT_ONLY` back or document/migrate it explicitly), and I'll do a full re-review at that point. -- 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]
