SEZ9 commented on PR #12578: URL: https://github.com/apache/seatunnel/pull/12578#issuecomment-6568193069
Thanks for the follow-up on the CI side. To summarize where I think we are: **CI / merge gate** The automated notes make the picture clear: the current head `ef53a70c6f72f5f6ae7c31fcc57b541eb3c849f3` shows the Notify test workflow (run 38787676286 / job 116603788523) as `FAILURE` with an `HttpError: Unexpected end of JSON input` in the Notify step, and labeler (run 38787676256 / job 116603786377) as `SUCCESS`, with no Build check reported on the Apache side. The matching fork Build run 39775915510 (attempt 3) is reported as `success` for the same SHA. I agree that the Notify failure looks like a dispatch/API parsing problem rather than anything related to the row-cache change, so I'm not asking you to push more commits or re-trigger runs just to chase it. I'll look at reconciling/adopting the matching fork run through the update-build-status mechanism from our side. No action needed from you on this. **Remaining review items** The CI discussion doesn't change the open review points, so I'd like to close those out before this can be approved. Could you reply on each, either with a change or a short rationale: 1. **PR12578-F1 (Performance, MEDIUM)** – `setField` in `seatunnel-api/src/main/java/org/apache/seatunnel/api/table/type/SeaTunnelRow.java` does an unconditional volatile write, which puts a memory barrier on every field assignment during row construction. Can this be made conditional (e.g. only write when the cached size is actually populated) or otherwise kept off the hot path? If you believe the cost is negligible, a quick benchmark or reasoning would help. 2. **PR12578-F2 (Docs, LOW)** – The invalidation contract currently lives only in a prose paragraph under "Source Contract" in `docs/en/architecture/core-api-design.md`. Please add Javadoc to `setField` describing the contract (what invalidates the cache and what does not), and reconsider the paragraph's placement so it matches its actual scope. 3. **PR12578-F3 / F5 (Test, LOW)** – `testCopyCachedSizeIndependentAfterSetField` in `SeaTunnelRowTest.java` doesn't exercise anything beyond the first test. Please add a case that pins the documented limitation: mutating a nested row / via `getFields()` does **not** invalidate the cached size, and a case showing the cache is shared between the two overloads. 4. **PR12578-F4 (Test, LOW)** – The regression test covers typed-then-typed and untyped-then-untyped on separate rows. The production flow is a typed pre-warm followed by untyped reads on the same row; please add that cross-overload case. Once those are in (or explained), I'm happy to take another look and sort out the check adoption. Thanks for your patience with the CI noise. <!-- streview-comment:2218 --> -- 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]
