SEZ9 commented on PR #11512: URL: https://github.com/apache/seatunnel/pull/11512#issuecomment-5658210694
Thanks for resolving the `dev` conflicts, @goutamadwant. I went through `097c716147` and `0a84c6b811` against the current head; the PR-owned changes there are `TaskExecutionService.java`, `TaskGroupContext.java`, `TaskExecutionServiceTest.java` and `CdcProgressServiceTest.java`. The execution-scoped cleanup fix looks like a genuine correctness improvement and the ordering coverage is welcome. On the freshness-window question: agreed. Age-boundary classification belongs with the freshness-policy consumer, and I'd rather not widen this PR's public contract for it. No further ask on that. That said, neither of the two new commits touches the files from my previous review, so those findings still stand as open. Concretely, what I'm still looking for: - **F1 / F7 / F8 – `CdcEnumeratorProgressReport.java`**: `activeSplits` needs an actual bound to match the documented "bounded active-split details" contract (a cap plus a documented truncation behaviour is fine); count fields should reject negative / mutually inconsistent values at construction; and either make `CdcSnapshotSplitProgress` genuinely immutable or drop the "immutable per-split details" wording, since the shallow copy alone doesn't guarantee it. - **F3 – `CdcProgressAccuracy.java` (and the other API enums in the report)**: please confirm the engine codec encodes these by name rather than ordinal. If it already does, a pointer to where is enough and I'll close this one. - **F2 / F4 / F5 – `docs/en/developer/cdc-progress.md`**: add an explicit rule that connector-native position payloads must not carry credentials or connection secrets; rewrite the runtime-collection section so it describes the registration-based enumerator report path that is actually implemented rather than a pull/derive model; and list which connectors currently implement the provider (MySQL today, with the `connector-cdc-base` note for the others). - **F6 – `CdcProgressLifecycle.java`**: trim the `SNAPSHOT` Javadoc so it no longer includes enumerator-owned discovery/assignment, to stay consistent with the ownership rule in the doc. If you'd prefer to split any of these into a follow-up, say which ones and why, and I'll consider it — but the bound on `activeSplits` (F1) and the enum-by-name encoding (F3) I'd like settled in this PR since they shape the wire contract. <!-- streview-comment:1028 --> -- 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]
