ryanmeowy commented on issue #11007:
URL: https://github.com/apache/seatunnel/issues/11007#issuecomment-6061145628
@DanielLeens Thanks for the correction — agreed, the single five-option
`conditional(...)` was wrong: it would make
`gtid-set`/`skip-events`/`skip-rows` mandatory and reject legitimate
file+pos-only anchors, and it would propagate into OceanBase through the shared
factory. Dropped.
Below is the minimal validation design you asked for — **connector-side
only: no OptionRule DSL changes, no runtime behavior changes, no new options**.
I won't open a PR until you confirm the design and pick the wrong-mode variant
(§5).
### 1. Where each rule is enforced today
Runtime (unchanged, `MySqlIncrementalSource.java`, dev @ 3ac7003fb):
- **C1** `mode=specific`: `file`+`pos` configured together (:107-114), both
required (:116-123), `file` not blank (:134-139)
- **C2** `gtid-set`, when present: not blank (:161-166), parseable GtidSet
(:168-177)
- **C3** `skip-events`/`skip-rows`: default 0, ≥ 0 (:181-188)
- **C4** none of the five `startup.specific-offset.*` options unless
`mode=specific` (:190-198)
`--check` (`SeaTunnelConfValidateCommand.java:307-332`, config-level by
design — see the comment at :203) only validates `factory.optionRule()` and
never instantiates the source, so today C1–C4 surface **only at runtime** —
e.g. `mode = INITIAL` + `specific-offset.file` passes `--check` and fails at
job submission.
### 2. Proposed declarative layer (factory only)
```java
// MySqlIncrementalSourceFactory#getOptionRuleBuilder
// startup.mode moves from the plain optional block to the constraint
variant (Variant A)
.optional(
MySqlIncrementalSourceOptions.STARTUP_MODE,
Conditions.extension(
MySqlIncrementalSourceOptions.STARTUP_MODE,
SPECIFIC_OFFSET_MODE_GUARD))
...
// file+pos required together under specific — same shape as the existing
STOP_MODE rule (:106-110)
.conditional(
MySqlIncrementalSourceOptions.STARTUP_MODE,
StartupMode.SPECIFIC,
SourceOptions.STARTUP_SPECIFIC_OFFSET_FILE,
SourceOptions.STARTUP_SPECIFIC_OFFSET_POS)
// same trigger → merged into the same sub-rule
(Builder#mergeConditionalRule);
// per-option constraints, so absent values skip evaluation
(isConstraintApplicable) —
// that is why each stands alone instead of one .and() chain
.conditional(
MySqlIncrementalSourceOptions.STARTUP_MODE,
StartupMode.SPECIFIC,
Conditions.notBlank(SourceOptions.STARTUP_SPECIFIC_OFFSET_FILE),
Conditions.notBlank(MySqlIncrementalSourceOptions.STARTUP_SPECIFIC_OFFSET_GTID_SET),
Conditions.greaterOrEqual(
MySqlIncrementalSourceOptions.STARTUP_SPECIFIC_OFFSET_SKIP_EVENTS, 0L),
Conditions.greaterOrEqual(
MySqlIncrementalSourceOptions.STARTUP_SPECIFIC_OFFSET_SKIP_ROWS, 0L))
```
```java
private static final ConditionExtension<StartupMode>
SPECIFIC_OFFSET_MODE_GUARD =
new ConditionExtension<StartupMode>() {
@Override
public String description() {
return "'startup.specific-offset.*' options can only be used
when "
+ "'startup.mode' is 'specific'";
}
@Override
public boolean evaluate(ReadonlyConfig config, StartupMode mode)
throws OptionValidationException {
if (mode == StartupMode.SPECIFIC ||
!anySpecificOffsetPresent(config)) {
return true;
}
throw new OptionValidationException(
String.format(
"'startup.specific-offset.*' options can
only be used when "
+ "'startup.mode' is 'specific', but
current mode is '%s'.",
mode));
}
};
```
- The five options **stay listed in the plain optional block** for
metadata/web-UI; conditional targets may overlap the optional list (same
pattern as `ElasticsearchSourceFactory`'s AUTH rules).
- `ConditionExtension` is used strictly through its documented contract
("cross-field checks"); `seatunnel-api` is not touched.
### 3. Why the wrong-mode rule needs `ConditionExtension`
`ConditionOperator` has no "must be absent" operator, and binary-literal
conditions require a non-null expected value, so "absent when mode ≠ specific"
is not expressible with built-ins. Rather than broaden the DSL, the guard is a
connector-local extension wired through the existing `.optional(option,
condition)` hook. `description()` is all that REST/metadata export serializes;
`evaluate()` throws with the same message text as the runtime check (:190-198),
so users see one consistent error.
### 4. Compatibility matrix
Invariant: **no config valid today becomes invalid and no invalid config
becomes valid; the design only advances a subset of already-invalid configs
from runtime failure to `--check` failure.**
| # | `startup.mode` | `startup.specific-offset.*` | dev (runtime) | after —
Variant A | after — Variant B |
|---|---|---|---|---|---|
| 1 | `specific` | `file`+`pos` (+ valid gtid-set, skips ≥ 0) | valid |
valid (unchanged) | valid |
| 2 | `specific` | only one of `file`/`pos` | rejected (:107-114) |
`--check`: pair rule | same |
| 3 | `specific` | none | rejected (:116-123) | `--check`: pair rule | same |
| 4 | `specific` | blank `file` | rejected (:134-139) | `--check`:
`notBlank(file)` | same |
| 5 | `specific` | blank `gtid-set` | rejected (:161-166) | `--check`:
`notBlank(gtid-set)` | same |
| 6 | `specific` | negative `skip-*` | rejected (:181-188) | `--check`:
`greaterOrEqual` (absent → skipped, so default-0 stays valid) | same |
| 7 | `specific` | malformed `gtid-set` | rejected (:168-177) | runtime-only
(unchanged — parse is not declarative) | same |
| 8 | `INITIAL`/`EARLIEST`/`LATEST`/`TIMESTAMP` | any of the five | rejected
(:190-198) | `--check`: guard | `--check`: guard |
| 9 | omitted (runtime default `INITIAL`) | any of the five | rejected |
**runtime (residual)** | `--check`: per-option guard |
| 10 | omitted | none | valid | valid | valid |
| 11 | invalid enum value | — | `--check` already rejects (`singleChoice`) |
unchanged | unchanged |
| 12 | stop-side rules (`STOP_MODE=specific` pair, `TIMESTAMP`,
`EXACTLY_ONCE`) | — | already declarative (:102-114) | untouched | untouched |
OceanBase inherits both layers with zero code change
(`OceanBaseIncrementalSourceFactory extends MySqlIncrementalSourceFactory`,
`OceanBaseIncrementalSource extends MySqlIncrementalSource`) — same matrix.
### 5. Wrong-mode coverage — your call
- **Variant A** (sketched above): guard on `startup.mode` only. Smallest
diff (one option moved to the constraint variant + one extension). Residual:
row 9 (mode omitted + offset present) still fails at runtime, not `--check` —
because `--check` validates raw presence and does not resolve `startup.mode`'s
default (`ReadonlyConfig#getOptional` vs `get`).
- **Variant B**: the same guard logic behind thin per-option adapters on
each of the five options (`extension(FILE, adapter)` …); `startup.mode` stays
untouched in the plain optional block. Presence of any specific-offset option
triggers the check, so row 9 also fails at `--check`. ~5 × 6 LOC more.
Both variants satisfy "no DSL broadening, no runtime changes". My suggestion
is **B** (row 9 is the realistic misconfiguration — a user adds
`specific-offset.file` and forgets `startup.mode`), but A is fine if you want
the smallest slice.
### 6. Tests & verification
- `MySqlIncrementalSourceFactoryTest`: matrix-driven cases — one
`ConfigValidator` assertion per row (error keys + message fragments), replacing
the single `assertNotNull`.
- OceanBase: one assertion that its inherited `optionRule()` carries the
pair + guard.
- `./mvnw spotless:apply`, module verify, then full `./mvnw -q -DskipTests
verify` before the PR.
- PR: `[Fix][Connector-V2][CDC] …` against `dev`, body links back here. No
label/assignment changes in this pass.
--
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]
