DanielLeens commented on PR #11029:
URL: https://github.com/apache/seatunnel/pull/11029#issuecomment-5611982347
Thanks @SEZ9. Checked the raw API body of review 5148548584 again — it is
not cut off, it runs the full five-section structure through "Overall
assessment" at the end of the Merge Recommendation. Same rendering/collapse
pattern we've seen elsewhere on long bodies.
Going through your open items against that full text, plus a direct source
check on this head (`f561cad5e13`) for the two your list flags that the review
text doesn't restate in prose:
- **F1 (snapshot-only)** — confirmed in section 1.1:
`OpengaussSourceOptions.java:50-56` includes `SNAPSHOT_ONLY` alongside
`INITIAL`/`EARLIEST`/`LATEST`, only `COMMITTED_OFFSET` excluded, pinned by
`testOptionRuleExposesOnlyOpengaussStartupModes`/`testOptionRuleOffersExactlyOnceForBothSnapshotModes`.
- **F2/F5 (replica-identity fail-fast + docs)** — covered in section 1.1
("Replica-identity error message" bullet: `PostgresDialect.java:140-141` now
names both remediations, `ALTER TABLE ... REPLICA IDENTITY FULL` or
`require-replica-identity-full=false`) and section 2.3 (`Opengauss-CDC.md`
en/zh updated to match the four valid `startup.mode` values). Both confirmed
fixed.
- **F3 (`getStartupModeOption()` init-order trap)** — covered in section
1.1: now documented in place at `OpengaussIncrementalSource.java:49-58` as a
hard constraint ("must never read instance state"). I independently re-fetched
that file at the current head and confirmed the javadoc is there verbatim,
right above the override.
- **F4 (serialized job DAG vs. hierarchy change)** — covered in section 1.2
(Compatibility Impact): `serialVersionUID` on
`PostgresIncrementalSource`/`PgBaseIncrementalSource`/`PgBaseSourceConfigFactory`
is pinned and cross-checked against real `serialver` output from published
2.3.12/2.3.13 artifacts, with an explicit note that a computed UID would drift
and break rolling upgrades. That's the right way to prove this is safe for
pre-upgrade serialized DAGs.
- **F6 (docs stating connector-owned `startup.mode` set)** — same as F2/F5's
doc half, confirmed in section 2.3.
- **F7 (raw `(SingleChoiceOption)` cast)** — not restated in the review
prose, so I went and re-pulled `OpengaussSourceOptions.java` at `f561cad5e13`
directly. It's fixed: `STARTUP_MODE` is declared straight from
`.singleChoice(StartupMode.class, ...)` with no cast, and there's now a javadoc
on the field explaining why none is needed ("the single-choice builder already
returns a `SingleChoiceOption` of the chosen enum type").
- **F8 ("Apache OpenGauss" wording)** — also re-checked directly:
`OpengaussIncrementalSource.java`'s class javadoc now reads "OpenGauss source
entry point that reuses the shared PG-base PostgreSQL implementation..." — no
"Apache OpenGauss" anywhere in the file. Fixed.
So all eight items (F1-F8) are confirmed fixed on `f561cad5e13`, F1-F6 from
the review text itself and F7/F8 from a direct re-check of source I just did.
On the one open item from my own review — `Build` was still `in_progress` on
this head when I posted — I just re-checked `gh pr checks`: it has since
completed `SUCCESS`. With CI green and all eight items confirmed on the same
head, I have no remaining findings here.
--
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]
