nzw921rx commented on PR #11023: URL: https://github.com/apache/seatunnel/pull/11023#issuecomment-4659287520
> Thanks for working on this. I re-reviewed the latest head from scratch again, and this time I specifically checked the current branch as it exists now, not the older revision that still experimented with the shared `OptionRule` change. > > # What This PR Fixes > * User pain: several connectors want to express richer validation such as “these options are mutually exclusive, but the chosen one must also be non-empty or satisfy numeric constraints”. > * Fix approach: the PR migrates multiple connector factories to the new `exclusive(...)` + constrained `optional(...)` style and adds factory tests around those rules. > * One-line summary: the connector-side direction is reasonable, but the current head still depends on a shared `OptionRule` contract that is not actually part of this branch anymore. > > # Runtime Chain Rechecked > ``` > connector factory startup > -> XxxIncrementalSourceFactory.optionRule() > -> OptionRule.builder() > -> exclusive(optionA, optionB) > -> optional(optionA, Conditions...) > -> ConfigValidator.validate(rule) > ``` > > # Key Findings > 1. The normal runtime path really does hit `optionRule()` during connector factory initialization. > 2. The current PR head no longer carries the shared `OptionRule` change that would relax duplicate registration across `exclusive(...)` and constrained `optional(...)`. > 3. Because of that, the connector migrations still rely on a contract that `upstream/dev` does not provide. > > # Findings > Issue 1: the migrated factories still build rules that the current shared `OptionRule` contract rejects > > * Location: `seatunnel-connectors-v2/connector-cdc/connector-cdc-mysql/src/main/java/org/apache/seatunnel/connectors/seatunnel/cdc/mysql/source/MySqlIncrementalSourceFactory.java:63-69`, `seatunnel-connectors-v2/connector-cdc/connector-cdc-mongodb/src/main/java/org/apache/seatunnel/connectors/seatunnel/cdc/mongodb/MongodbIncrementalSourceFactory.java:65-73`, plus the same pattern in the Oracle/Postgres/SQL Server factory updates. > * Why this is a blocker: > the branch now does `exclusive(TABLE_NAMES, TABLE_PATTERN)` and then `optional(TABLE_NAMES, Conditions.notEmpty(...))` (same for the other connectors), but the shared `OptionRule` on `dev` still treats that as a duplicate option registration. So the latest connector code is still depending on a builder behavior that is not present on the current base branch. > * Better fix: > either bring back the shared `OptionRule` support change in the same PR, or express the connector-side validation in a way that works with the current `OptionRule` contract. > * Severity: High > * Already raised by others: No > > Issue 2: the new tests instantiate the broken rule eagerly, so they do not protect the intended migration contract > > * Location: `seatunnel-connectors-v2/connector-cdc/connector-cdc-mysql/src/test/java/org/apache/seatunnel/connectors/seatunnel/cdc/mysql/source/MySqlIncrementalSourceFactoryTest.java:35-38`, `seatunnel-connectors-v2/connector-cdc/connector-cdc-mongodb/src/test/java/mongodb/source/MongodbIncrementalSourceFactoryTest.java:41-45` > * Why this is a blocker: > both tests initialize `private final OptionRule rule = new ...Factory().optionRule();`. If the shared builder contract still rejects the duplicate registration pattern above, the tests are not validating a working migration path; they are just tightly coupled to a branch-local assumption. > * Better fix: > fix the shared contract mismatch first, then keep these tests as focused validation for the intended option-rule behavior. > * Severity: Medium > * Already raised by others: No > > ### Conclusion: can merge after fixes > 1. Blocking items > > * Issue 1: the connector migration cannot land until the shared `OptionRule` contract it depends on is present on this branch as well. > * Issue 2: keep the factory tests, but only after the builder contract mismatch is resolved. > > 2. Suggested non-blocking follow-up > > * None beyond the two items above. > > Overall, I like the validation direction, but the current head is still split between connector-side usage and base-contract support, so I would not merge it as-is. @DanielLeens Thank you for your review. The dev branch already supports it. Please use the latest code. -- 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]
