DanielLeens commented on PR #11512: URL: https://github.com/apache/seatunnel/pull/11512#issuecomment-5473717562
Thanks for the re-check, @SEZ9. I looked closely at this pass because all 8 items you listed as "still requiring follow-up" are ones I had already reviewed as fixed on this exact PR — 6 of them (Issues 1, 2, 4, 5, 6, 7) in my 2026-08-22 review, and the remaining 2 (Issue 3 / your "codec must encode by name" and Issue 8 / the immutability item) in my 2026-08-24 review. Both of those reviews predate the head you checked here (`b1a403bdbb3e`, 2026-08-29), so I re-verified all 8 directly against the source at that exact commit rather than relying on my own prior notes, and every one is present in the code today: - **Issue 1 (unbounded `activeSplits`)**: `CdcEnumeratorProgressReport.java:39` defines `public static final int MAX_ACTIVE_SPLITS = 100`, and the constructor caps the retained list and sets `activeSplitsTruncated` (lines 65-66, 86, 126-131). Covered by `testEnumeratorReportBoundsActiveSplitDetails`. - **Issue 2 (credentials in position payloads)**: `docs/en/developer/cdc-progress.md:38` already states payloads "contain credentials, connection URLs, or other authentication material" are forbidden (mirrored in the zh doc and `CdcProgressPosition` Javadoc). - **Issue 3 (ordinal vs. name encoding)**: `CdcProgressReportSerializer.java` encodes every enum via `.name()` and decodes via `.valueOf(...)` (lines 49, 109, 129, 209, 213, 218, 222, 257) — no `.ordinal()` usage anywhere in the file. Four exhaustive round-trip tests (`testEveryProgress*RoundTripsByName`) pin this per-constant. - **Issue 4 (docs describing a pull/derive model)**: `docs/en/developer/cdc-progress.md` (Runtime collection section) already reads "Enumerator report sources are registered when coordinator task groups are deployed... requests reports from the registered member, or updates the coordinator-local report directly when the enumerator runs on the master" — this matches the registration-based implementation, not the old pull/derive wording. - **Issue 5 (no stated connector coverage)**: `docs/en/developer/cdc-progress.md`'s "Current limitations" section already has the bullet: "CDC sources based on `connector-cdc-base` currently provide reports... CDC sources without this provider wiring return no report." - **Issue 6 (`SNAPSHOT` Javadoc conflating reader/enumerator ownership)**: `CdcProgressLifecycle.java:25` already reads "The reader is reading snapshot splits." — the discovery/assignment wording is gone. - **Issue 7 (no invariant validation on counts)**: `CdcEnumeratorProgressReport.java:118-123` calls `validateCount(...)` for every count field and `validateExactSplitCounts(...)`, both throwing `IllegalArgumentException` (lines 170-185). Covered by `testEnumeratorReportRejectsInvalidCounts`. - **Issue 8 (element-level immutability of `CdcSnapshotSplitProgress`)**: the class is already `public final class` with all-`final` fields (`CdcSnapshotSplitProgress.java:33, 36, 39, 42, 45`), and the class Javadoc (lines 28-29) now states explicitly: "This value is deeply immutable: its fields are final, `CdcProgressValue` is immutable, and any contained [position] defensively copies...". Since every item checks out against the actual current-head source rather than just my earlier notes, my read is that this pass may have re-posted the outstanding-issue list from your 2026-08-21 review without re-diffing against the fixes that landed in between (they were all in place well before `b1a403bdbb3e`) — happy to be corrected if you're seeing something different in the code itself, in which case a `path:line` pointer at the current head would help me find it. From my side, the merge recommendation stands as I posted on 2026-08-29: no open source-level issue, with the only remaining gate being a completed green CI run on this head plus the formal maintainer approval (I only have comment access 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]
