DanielLeens commented on PR #12578: URL: https://github.com/apache/seatunnel/pull/12578#issuecomment-6574857967
Thanks @SEZ9 for the summary. One note on the four review items in your last comment: they read like the original wording from the first round, and they no longer match the unchanged head `ef53a70c6f72f5f6ae7c31fcc57b541eb3c849f3`. I re-checked that head by code reading only (nothing executed locally), and the head has not moved since the items were first raised: 1. **F1 (volatile write):** `SeaTunnelRow.setField` (`SeaTunnelRow.java:52-60`) assigns the field and writes `size = 0` only under `if (size != 0)`. The unconditional volatile write on the row-construction path is gone; the cost left on that path is a single volatile read. 2. **F2 (Javadoc and placement):** `setField` now has Javadoc stating that it clears the cached estimate and that direct writes through `getFields()` and nested-object mutations bypass invalidation. The EN/ZH paragraph sits under "Row Data Contract" (`docs/en/architecture/core-api-design.md` around lines 177-181), not under a "Source Contract" heading. 3. **F3/F5 (limitation pinned):** `SeaTunnelRowTest#testDirectFieldMutationDoesNotInvalidateCachedBytesSize` (`:280`) and `#testNestedFieldMutationDoesNotInvalidateParentCachedBytesSize` (`:297`) assert the stale cached size is still returned after a `getFields()` write or a nested mutation, then assert that calling the parent `setField` recomputes it. 4. **F4 (cross-overload on one row):** `#testSetFieldInvalidatesCachedBytesSize` (`:253`) does typed pre-warm, `setField`, untyped read, and then the reverse on a fresh row. Note that `testCopyCachedSizeIndependentAfterSetField`, which your item 3 mentions, is not a test name in the test class at this head, so that item may be pointing at an earlier revision. So I believe all four points are already addressed at this head, and @hutiefang76 does not need to push anything further for them. If you read the diff and still see a gap in one of them, please quote the specific line so the contributor can act on it. My source-level conclusion is unchanged: no source-side blocker. On CI: the apache-side head rollup still shows `Notify test workflow` as `FAILURE` and `labeler` as `SUCCESS`, with no `Build` check attached. The matching fork Build run (https://github.com/hutiefang76/seatunnel/actions/runs/39775915510, attempt 3) is reported successful for the same SHA. The head is 157 commits behind `dev`, but I am not requesting a sync, since the open gate is the check adoption you mentioned and not a code or merge-base problem. -- 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]
