SEZ9 commented on PR #11512: URL: https://github.com/apache/seatunnel/pull/11512#issuecomment-5230722445
Thanks for the updates @goutamadwant, and thanks @nzw921rx for the detailed write-up — happy to discuss. **Build/CI:** Glad the dev sync issues are resolved and code style CI is green. The local 364-test engine-server run plus the E2E compile check covers what I was worried about. **HybridSplitAssigner:** Thanks for re-checking against dev. You're right — I re-diffed and the change is purely additive (implementing the new interface plus `getCdcEnumeratorProgress(...)`), which is source- and binary-compatible for external implementations. My earlier concern was based on an intermediate revision; consider it resolved. **Deserialization:** The explicit wire codec in `ReportCdcProgressOperation` is exactly what I was asking for, and the added coverage for malformed collection counts and empty batches is appreciated. **Docs:** Agreed there's no config surface to document. However, this PR does add a user-facing capability — operators need to know how to actually see these reports. Two concrete asks: (1) make sure all new public API types carry the `@Experimental` annotation, and (2) add a short docs section (or at minimum a note in the STIP-30 tracking issue) describing how the retained per-task reports are exposed/consumed. If they're engine-internal only for now with exposure coming in a follow-up, say so explicitly in the Javadoc so connector authors don't build against assumptions. **On @nzw921rx's consolidation proposal:** I think the analysis is directionally right. Reader and enumerator *reports* should stay distinct types — they own different facts, and merging them would blur accuracy semantics. But the engine-side plumbing (envelope, operation payload, sequence ordering, latest-report storage in `CdcProgressService`) is genuinely duplicated and will be painful to keep in sync. My preference: keep `CdcReaderProgressProvider`/`CdcEnumeratorProgressProvider` as-is on the connector-facing side (the split avoids type-erasure awkwardness in provider discovery), but collapse the transport/storage path into a single tagged envelope and one ordering/storage map keyed by role. Since the API is experimental, now is the cheapest time to do this — I'd rather not defer it. @goutamadwant if you can address the docs/`@Experimental` items and the engine-side consolidation, I think this is close. Naming can stay as-is; the current names are consistent with what the types actually represent. <!-- streview-comment:112 --> -- 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]
