SEZ9 commented on PR #11512: URL: https://github.com/apache/seatunnel/pull/11512#issuecomment-5770496503
Thanks for the follow-up, @goutamadwant, and for the test summary (582 on JDK 11, 64 focused on JDK 8, two existing benchmark skips in each run). One thing I need before I can mark items resolved: your comment refers to "the three Medium requests" and "Low 6–8", but the open items I'm tracking on this PR are the eight findings below (six Medium, two Low), so I can't map your summary onto them with confidence. Could you reply with a per-finding pointer (commit, or file/class) for each one, or say explicitly which ones you consider out of scope and why? - **F1 (Medium, Robustness)** – `CdcEnumeratorProgressReport.activeSplits` is unbounded while the contract says "bounded active-split details". You mention "bounded reader-position publication"; is the *enumerator* active-split list also capped now? If so, what is the cap, and what does the report carry when it is exceeded (truncated list plus the full count)? - **F2 (Medium, Security)** – `docs/en/developer/cdc-progress.md` should state that connector-native position payloads shipped to and stored on the coordinator must not contain credentials or connection secrets. Please point me to the wording. - **F3 (Medium, Compatibility)** – the engine codec must encode the public API enums (e.g. `CdcProgressAccuracy`) by name, not ordinal. You mention "serialization coverage"; does that include a round-trip test that would fail if the encoding were ordinal-based? - **F4 (Medium, Docs)** – the runtime-collection section still described a pull/derive model rather than the registration-based enumerator report path. Has that section been rewritten? - **F5 (Medium, Docs)** – the developer doc should name which connectors currently implement the progress provider. - **F6 (Medium, Docs)** – `CdcProgressLifecycle.SNAPSHOT` Javadoc included enumerator-owned discovery/assignment, contradicting the ownership rule in the doc. - **F7 (Low, Robustness)** – count fields accept negative and mutually inconsistent values with no invariant validation. - **F8 (Low, Robustness)** – the "immutable per-split details" claim relies on a shallow copy; element immutability of `CdcSnapshotSplitProgress` is not guaranteed by the report class. "Immutable samples" sounds like it may cover this – please confirm where. Once I have that mapping I'll re-check `dfabd777e98` (or whatever head you point me at) against these eight items only and close out whatever is done. Thanks again for the quick turnaround. <!-- streview-comment:1225 --> -- 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]
