SEZ9 commented on PR #11512: URL: https://github.com/apache/seatunnel/pull/11512#issuecomment-6104757287
Thanks for syncing the branch with `dev`. The automated re-check of head `9cf8e059b8` (merging `0278a6a74` on top of `44d6d665a`) confirms the net PR diff is unchanged apart from import ordering in `TaskExecutionServiceTest.java` and that the earlier conflict is resolved correctly, so the merge itself looks good to me. That merge did not touch the points from my previous review, though, so they are still open. Before I can approve, could you either address or explicitly respond to each of these: - **PR11512-F1 (Robustness, `CdcEnumeratorProgressReport`)** – `activeSplits` still accepts an unbounded list while the doc promises "bounded active-split details". Please enforce a cap (or truncate with a flag) so a connector cannot exhaust coordinator memory/transport. - **PR11512-F2 (Security, `docs/en/developer/cdc-progress.md`)** – the payload contract should explicitly forbid credentials/connection secrets in connector-native position payloads, since those are shipped to and stored on the coordinator. - **PR11512-F3 (Compatibility, `CdcProgressAccuracy` and the other public enums in the wire payload)** – please confirm the engine codec encodes these by name rather than ordinal, and point me to the test covering it. - **PR11512-F4 (Docs)** – the runtime-collection section still describes a pull/derive model; it should describe the registration-based enumerator report path this PR implements. - **PR11512-F5 (Docs)** – please state which connectors currently implement the progress provider. - **PR11512-F6 (Docs, `CdcProgressLifecycle`)** – the `SNAPSHOT` Javadoc still includes enumerator-owned discovery/assignment, which contradicts the ownership rule in the new doc. - **PR11512-F7 (Robustness)** – count fields accept negative/mutually inconsistent values; a constructor-level invariant check would be enough. - **PR11512-F8 (Robustness)** – the "immutable per-split details" claim relies on a shallow copy; either make `CdcSnapshotSplitProgress` genuinely immutable or soften the Javadoc. If any of these were already handled in a commit I missed, just point me at it and I'll re-check. Once F1–F8 are resolved I'm happy to do a final pass. <!-- streview-comment:1664 --> -- 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]
