DanielLeens commented on PR #11512:
URL: https://github.com/apache/seatunnel/pull/11512#issuecomment-5662012101
Thanks @SEZ9 — since you flagged F1/F3/F7/F8 as touching files my last full
review didn't cover (`CdcEnumeratorProgressReport.java`,
`CdcProgressAccuracy.java`, `CdcSnapshotSplitProgress.java`,
`docs/en/developer/cdc-progress.md`), I went and read all of them directly
against the current head (`0a84c6b811419fad551840e07d88bb8f49157945`) rather
than assume they're unchanged. Several of these look already resolved — here's
what I found, with pointers so you can verify quickly:
**F3 (enum-by-name encoding)** — confirmed by name, not ordinal.
`CdcProgressReportSerializer.java`: `writeAccuracy` writes `accuracy.name()`
(`:218`) and `readAccuracy` reads it back via
`CdcProgressAccuracy.valueOf(in.readString())` (`:221-222`); same pattern for
`envelope.getOwner().name()` (`:49`),
`report.getSnapshotAssignmentStatus().name()` (`:109`), and `lifecycle.name()`
(`:209`). Every enum in this wire format goes by name.
**F1 (activeSplits bound + count validation + immutability)** — looks fully
resolved already. `CdcEnumeratorProgressReport.java`:
- Bound: `MAX_ACTIVE_SPLITS = 100` (`:39`), enforced in the constructor via
`Math.min(splitDetails.size(), MAX_ACTIVE_SPLITS)` (`:126`) with an
`activeSplitsTruncated` flag (`:65-66`, `:130-131`) that's documented and
exposed via `isActiveSplitsTruncated()`.
- Negative/inconsistent counts: `validateCount` rejects any negative value
(`:170-174`), and `validateExactSplitCounts` (`:176-187`) rejects
`EXACT`-accuracy `assigned != completed + running`.
- Immutability: the list is defensively copied and wrapped
(`Collections.unmodifiableList(new ArrayList<>(splitDetails.subList(...)))`,
`:127-129`), and `CdcSnapshotSplitProgress` itself
(`CdcSnapshotSplitProgress.java`) is a genuinely immutable value type —
all-final fields, no setters, constructor-time `Objects.requireNonNull` on
every field, and the class Javadoc (`:24-31`) states exactly why it's deeply
immutable rather than just claiming it.
**F5/F2 (docs: connector list + secrets rule)** — both already present in
`docs/en/developer/cdc-progress.md`: the connector list is in the "Current
limitations" section ("CDC sources based on `connector-cdc-base` currently
provide reports. MySQL uses an explicit `MYSQL_BINLOG` position family; other
base connectors use their plugin name...", `:71-73`), and the secrets rule is
in "Provider contract" ("Position payloads must contain only offset
coordinates... They must never contain credentials, connection URLs, or other
authentication material.", `:36-38`).
**F6 (`CdcProgressLifecycle` SNAPSHOT javadoc)** — already trimmed. Current
text is just `/** The reader is reading snapshot splits. */` — no
enumerator-owned discovery/assignment language present.
**F4 (runtime-collection description: push/registration vs. pull/derive)** —
this is the one where I think the current doc text is actually already accurate
to the implementation, and I'd push back gently rather than treat it as open. I
traced the real mechanism: `CoordinatorService.collectCdcEnumeratorProgress()`
(`CoordinatorService.java:2193` onward) derives `taskGroupLocations` from the
active coordinator's own tracked task groups, then sends a
`CollectCdcEnumeratorProgressOperation` to each relevant member and folds the
returned `CdcProgressReportBatch` into `CdcProgressService.updateReports(...)`
(`:2238-2249`). `TaskExecutionService.collectEnumeratorCdcProgress(...)`'s own
Javadoc (`TaskExecutionService.java:1053`) literally says "Collects enumerator
reports **requested by** the active coordinator." That's a genuine
coordinator-initiated pull, not a worker-side registration/push — so
`cdc-progress.md:43-46` ("The active coordinator derives enumerator task group
location
s... requests reports from the assigned members...") reads to me as the
accurate description of what's actually implemented, not a mismatch. If you
were looking at an earlier revision of the collection path, happy to be pointed
at what changed; from current source I don't see the pull/derive wording as
wrong.
Given F1/F2/F3/F5/F6 all check out against current source and F4 looks like
the docs already match the real (pull-based) mechanism, the main open item from
your list is just confirming with @goutamadwant whether any of this was
intentional or needs a source-comment pointer added for future readers —
nothing here looks like a live defect to me on this pass.
Separately, a CI note since it's part of what I'm tracking on this PR: the
fork's `Build` run for this head (`0a84c6b811`) has since completed and shows
`engine-v2-it (8, ubuntu-latest)` and a few
`all-connectors-it`/`transform-v2-it` shards as failed. I pulled the
`engine-v2-it (8)` job log directly rather than going off the summary — the
actual failure is a Maven dependency-resolution error unrelated to any test:
`Could not transfer artifact org.postgresql:postgresql:jar:42.4.3 from/to
central ... Connection reset` while building `connector-jdbc`, which this PR's
diff doesn't touch. That's a transient Maven Central connectivity flake, not a
regression from this PR's `TaskExecutionService`/`TaskGroupContext` changes.
--
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]
