DanielLeens commented on PR #11512:
URL: https://github.com/apache/seatunnel/pull/11512#issuecomment-5633132567
@SEZ9 Let me ground every one of F1-F8 directly in this PR's own diff at
`b921264bb490` rather than in a description of current source state, since
that's the fair distinction you're drawing.
**F8 first, since you flagged it as cut off** — same recurring GitHub "show
more" collapse I've hit on other threads, not an actual truncation; here is the
full conclusion again in one piece:
Yes, the deep-immutability guarantee holds at every layer, verified against
the actual (unchanged) production code, not just the new test:
- `CdcProgressPosition`'s constructor does a real defensive copy —
`Collections.unmodifiableMap(new LinkedHashMap<>(values))` — so mutating the
caller's original map after construction cannot reach the stored copy, and the
returned view itself rejects writes.
- `CdcSnapshotSplitProgress` is `public final class` with four `final`
fields and no setters — no mutation path exists post-construction regardless of
what's passed in.
- `CdcEnumeratorProgressReport`'s active-splits list is wrapped the same way
(`Collections.unmodifiableList`), and
`testEnumeratorReportActiveSplitWatermarksAreDeeplyImmutable` exercises the
full nested chain (`CdcEnumeratorProgressReport` -> `CdcSnapshotSplitProgress`
-> `CdcProgressValue<CdcProgressPosition>` -> the map), not just the outer
list. I confirmed the test is discriminating (it would fail if the defensive
copy were removed) rather than vacuously true.
**F4 — the actual diff hunk**:
`seatunnel-engine/.../CoordinatorService.java`'s hunk is `@@ -2171,6 +2179,118
@@`, i.e. a 118-line addition starting at new-file line 2179;
`collectCdcEnumeratorProgress()` itself starts at line 2190, entirely inside
that added block — none of it is pre-existing code being described after the
fact. The doc side is the same: `docs/en/developer/cdc-progress.md` is a wholly
new file in this PR (`--- /dev/null`), and its "Runtime collection" section,
including the "derives enumerator task group locations from running job plans
and coordinator-owned slot assignments" sentence, is added at that file's own
diff (around line 46-50 of that file's hunk). So both halves of the
phrase-for-phrase match I described are new content introduced by this PR, not
a pointer into unrelated pre-existing code.
**On F1, F2, F3, F5, F6, F7 — same grounding check, since I understand why
you want it explicit:** `CdcEnumeratorProgressReport.java`,
`CdcProgressLifecycle.java`, `CdcSnapshotSplitProgress.java`,
`CdcProgressReportSerializer.java`, and `docs/en/developer/cdc-progress.md` are
**all wholly new files added by this PR** (each shows `--- /dev/null` in the
diff). There is no "pre-existing content" question for any of them — every line
in those files, including everything I cited earlier, is this PR's own diff by
construction. Concretely, within that diff:
- **F1**: `MAX_ACTIVE_SPLITS = 100` and the `activeSplitsTruncated`
field/constructor logic are new lines in `CdcEnumeratorProgressReport.java`'s
own diff, plus the matching doc sentence ("Enumerator reports retain at most
100 active-split details...") in the new `cdc-progress.md`.
- **F2**: the "must not contain credentials, connection URLs, or other
authentication material" sentence is a new line in `cdc-progress.md`, mirrored
in a new Javadoc line on `CdcProgressPosition.java`.
- **F3**: `CdcProgressReportSerializer.java`'s
`writeInternal`/`readInternal` pairs use `.name()`/`.valueOf(...)` throughout
(I independently counted the occurrences directly in the diff hunk); there is
no `.ordinal()` call anywhere in that new file.
- **F5**: the "CDC sources based on `connector-cdc-base` currently provide
reports... CDC sources without this provider wiring return no report" sentence
is a new line in `cdc-progress.md`.
- **F6**: `CdcProgressLifecycle.java`'s `SNAPSHOT` constant Javadoc ("The
reader is reading snapshot splits.") is a new line in that new file.
- **F7**: `validateCount(...)`/`validateExactSplitCounts(...)` are new
methods in `CdcEnumeratorProgressReport.java`, and
`CdcProgressModelTest.testEnumeratorReportRejectsInvalidCounts` (new test file,
`seatunnel-api/src/test/java/.../CdcProgressModelTest.java:155-180` in the
current head) exercises both the negative-count and the completed-exceeds-total
cases via the constructor directly.
So all eight are resolved, and now grounded against the diff itself rather
than a restated description of it.
**One new gate fact worth flagging separately, since it isn't a source
item**: I just rechecked the live merge state on this unchanged head
(`b921264bb490`) and it has flipped to a real conflict —
`mergeStateStatus=DIRTY`, `mergeable=CONFLICTING` against the current `dev`
(the PR is now `diverged`, `ahead_by=36`, `behind_by=85`). @goutamadwant, this
will need a sync with the latest `dev` and a conflict resolution before this
can move forward, independent of everything above. Separately, the current
`Build` failure (`rocketmq-connector-it (11, ubuntu-latest)`) is the same
pre-existing RocketMQ broker double-identity flake tracked by still-open PR
#12115, not something in this PR's 51-file diff — no action needed on that
specific job, but the merge conflict does need to be resolved and CI rerun
afterward regardless.
--
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]
