nzw921rx commented on PR #11023: URL: https://github.com/apache/seatunnel/pull/11023#issuecomment-4843520626
> Thanks for the update. I re-reviewed the latest head from the declarative CDC option-rule path down to the actual startup/stop offset consumption path again. > > ## What this PR solves > * User pain: several CDC connectors still reject invalid configs late, from scattered runtime `if/throw` checks instead of at submit time. > * Fix approach: move those checks into declarative `OptionRule + Conditions + ConditionExtension` across the CDC factories. > * One-line summary: the migration direction is still right, but the current head still leaves the Oracle / SQL Server offset-mode contract out of sync with the real runtime path. > > ## Runtime path I checked > ``` > Job submission > -> OracleIncrementalSourceFactory.optionRule() > -> SqlServerIncrementalSourceFactory.optionRule() > -> startup-mode / stop-mode dependent fields should be validated here > > Runtime consumption path > -> startup config / stop config resolution on the CDC source side > -> SPECIFIC / TIMESTAMP modes consume their companion offset/timestamp fields directly > ``` > > ## Findings > ### Issue 1: the latest head still does not declare the mode-dependent offset fields required by the real startup/stop runtime path > * Location: > > * `seatunnel-connectors-v2/connector-cdc/connector-cdc-oracle/src/main/java/org/apache/seatunnel/connectors/seatunnel/cdc/oracle/source/OracleIncrementalSourceFactory.java` > * `seatunnel-connectors-v2/connector-cdc/connector-cdc-sqlserver/src/main/java/org/apache/seatunnel/connectors/seatunnel/cdc/sqlserver/source/SqlServerIncrementalSourceFactory.java` > * Why this is a blocker: > On the current head, the factories only declare conditional validation for `STARTUP_MODE=TIMESTAMP` and `STARTUP_MODE=INITIAL`. The same option rules still do not cover the companion fields for the other offset-driven modes on the real runtime path, including the `SPECIFIC` startup case and the stop-mode paths that consume explicit timestamp/file/position values. > * Risk: > An incomplete offset-mode config can still pass submit-time validation and only fail later when the CDC source actually tries to build the startup/stop offset. > * Best fix: > Add the missing mode-specific conditional requirements to the Oracle and SQL Server factories, and add focused negative tests that prove those invalid offset-mode shapes are rejected during `ConfigValidator.validate(...)`. > * Severity: High > > ## Current conclusion > I still see a source-level blocker on the current head. The PR is closer, but the declarative validation contract is not yet fully aligned with the real runtime offset path. > > ### Conclusion: can merge after fixes > 1. Blocking items > > * Issue 1: close the remaining startup/stop offset-mode validation gap in the Oracle / SQL Server factories. > > 2. Suggested non-blocking follow-up > > * None from this round. @DanielLeens Thank you for your review. Please pull the latest code for review. Startup Mode and Stop Mode have been added -- 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]
