DanielLeens commented on PR #11512: URL: https://github.com/apache/seatunnel/pull/11512#issuecomment-5645058034
@SEZ9 good question, and no overlap — Issue 1 does not touch any of F1-F8. Your eight findings are all about the API/doc layer (report shape, immutability, doc accuracy). Issue 1 is a transport-layer concern one level down: the worker-side RPC that ships reader reports to the master. Different file, different failure mode, nothing in common. For the record, here's what Issue 1 actually flagged: `TaskExecutionService.reportCdcProgressToMaster()` (`TaskExecutionService.java:1010-1018`) issues a synchronous `nodeEngine.getOperationService().createInvocationBuilder(...).invoke().get()` with no explicit timeout, called from `reportReaderCdcProgress()` -> `reportCdcProgress()`, which runs at the tail of `updateMetricsContextInImap()` (`TaskExecutionService.java:872-916`). That method is scheduled via `scheduledExecutorService = Executors.newSingleThreadScheduledExecutor()` with `scheduleAtFixedRate(this::updateMetricsContextInImap, ...)` (`TaskExecutionService.java:262-266`) — the worker's one dedicated metrics-backup thread, used for nothing else. If the master is slow/GC-paused/partitioned, this blocks that thread for up to Hazelcast's default 60s operation-call-timeout, and since it's a single-thread `scheduleAtFixedRate`, that stalls every subsequent tick of the pre-existing job-metrics-to-IMap backup too — a new, b est-effort/experimental feature coupling itself to an existing reliability signal. I independently re-traced this exact chain against the current head (`5f04cb68f4`) just now and confirm it's accurate: the blocking `.invoke().get()` call, the single-thread executor, and the unconditional `reportCdcProgress()` call at the end of `updateMetricsContextInImap()` are all exactly as described. It's caught by a broad `try/catch` in `reportReaderCdcProgress()` so it won't crash the task, but the blocking wait happens before any exception is even possible. Now, status on your eight — I independently re-verified each against `5f04cb68f4` source rather than just relaying my own prior review: - **F1 (bounded `activeSplits`)** — Fixed, and it's real enforcement, not just documentation. `CdcEnumeratorProgressReport`'s constructor now computes `retainedSplitCount = Math.min(splitDetails.size(), MAX_ACTIVE_SPLITS)`, truncates via `subList(0, retainedSplitCount)` wrapped in `Collections.unmodifiableList`, and sets `activeSplitsTruncated` when the input exceeded the cap. There's no path to construct the report with more than 100 entries in `activeSplits`. - **F2 (credentials in position payloads)** — Fixed. `docs/en/developer/cdc-progress.md`'s Provider-contract section now says explicitly: "Position payloads must contain only offset coordinates such as binlog positions, GTIDs, LSNs, or timestamps. They must never contain credentials, connection URLs, or other authentication material." `CdcProgressPosition`'s class Javadoc carries the same constraint. - **F3 (name-based enum encoding)** — Confirmed again myself: `CdcProgressReportSerializer.java` has zero `.ordinal()` calls; every enum (`CdcProgressOwner`, `CdcSnapshotAssignmentStatus`, `CdcProgressLifecycle`, `CdcProgressAccuracy`) round-trips via `.name()` / `.valueOf(...)`. - **F4 (pull vs. push docs)** — Fixed, and the doc text now matches the real path. The "Runtime collection" section: "Reader reports are sampled on execution members, batched, and sent to the active coordinator... Enumerator reports use a separate coordinator-owned collection path." That's the actual split — reader side pushes, enumerator side is coordinator-pulled — not the old blanket "pull on demand" framing. - **F5 (connector listing)** — Fixed. "Current limitations" now states: "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... CDC sources without this provider wiring return no report." - **F6 (`SNAPSHOT` Javadoc scope)** — Fixed. `CdcProgressLifecycle.SNAPSHOT`'s Javadoc is now just "The reader is reading snapshot splits" — no enumerator-owned discovery/assignment language. That's cleanly a separate concept now (`CdcSnapshotAssignmentStatus` with its own `DISCOVERING`/`ASSIGNING`/`COMPLETED`), and the doc's Lifecycle section calls this out explicitly too. - **F7 (count validation)** — Fixed. `validateCount(...)` rejects negative values, and `validateExactSplitCounts(...)` throws `IllegalArgumentException` when all three counts are `EXACT` but `assigned != completed + running`. - **F8 (deep immutability)** — Fixed, and it's now a real, checkable guarantee rather than an assertion. `CdcSnapshotSplitProgress`'s Javadoc states the deep-immutability claim explicitly, and I traced the chain myself: `CdcProgressValue` has only final fields and no mutators; `CdcProgressPosition` defensively copies its input map into a new `LinkedHashMap` and wraps it in `Collections.unmodifiableMap` at construction. Nothing in the chain is mutable after construction. So: all eight of your findings check out as resolved on the current head. The one thing still open on this PR is Issue 1 above (transport-layer blocking RPC) — unrelated to your list, raised for the first time in my last review, not yet addressed in any commit. -- 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]
