SEZ9 commented on PR #11512: URL: https://github.com/apache/seatunnel/pull/11512#issuecomment-5650461130
Thanks for the update, @goutamadwant. On the current head `6139cfcba6b`, the delta since `5f04cb68f4` is confined to `TaskExecutionService.java` and the new `CdcReaderProgressReportingTest.java`, so none of the files from my earlier findings have moved. Those items therefore still stand as open. To keep this moving, here is what I'd like to see before I can approve: **Robustness — `seatunnel-api/src/main/java/org/apache/seatunnel/api/cdc/CdcEnumeratorProgressReport.java`** - F1: `activeSplits` still accepts an unbounded list while the docs promise "bounded active-split details". Please enforce a cap in the report (or the producer) and document the bound, so a large table set can't exhaust coordinator memory/transport. - F7: count fields accept negative and mutually inconsistent values. A small invariant check in the constructor/builder (non-negative, totals ≥ sub-counts) would be enough. - F8: the "immutable per-split details" claim relies on a shallow copy. Either make `CdcSnapshotSplitProgress` itself immutable and state that, or soften the Javadoc claim. **Security / Compatibility** - F2 (`docs/en/developer/cdc-progress.md`): please add an explicit rule that connector-native position payloads must not carry credentials or connection secrets, since they are shipped to and retained on the coordinator. - F3 (`seatunnel-api/src/main/java/org/apache/seatunnel/api/cdc/CdcProgressAccuracy.java`): confirm the engine codec encodes the new enums by name rather than ordinal, ideally with a test that decodes a report after a hypothetical enum reordering. **Docs** - F4: the runtime-collection section still describes a pull/derive model, whereas the implementation is registration-based enumerator reporting. Please align the text with what is actually wired. - F5: state which connectors implement the progress provider in this PR. - F6 (`seatunnel-api/src/main/java/org/apache/seatunnel/api/cdc/CdcProgressLifecycle.java`): the `SNAPSHOT` Javadoc includes enumerator-owned discovery/assignment, which contradicts the ownership rule in the doc — pick one and make them consistent. None of these require changes to checkpoint state or the public CDC model beyond the API classes listed above. Once these are pushed, ping me and I'll do a focused pass on just these points. <!-- streview-comment:1007 --> -- 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]
