test(amber): add unit test coverage for NetworkOutputBuffer#4958
test(amber): add unit test coverage for NetworkOutputBuffer#4958aglinxinyuan wants to merge 4 commits intoapache:mainfrom
Conversation
Pin the per-receiver batched-tuple sender used by every concrete Partitioner subclass: - Construction: batchSize defaults to ApplicationConfig.defaultDataTransferBatchSize; `to` and `dataOutputPort` are exposed as immutable accessors; an empty buffer at construction does not implicitly auto-flush. - addTuple: does not flush below batchSize; auto-flushes exactly at the batchSize boundary; produces one DataFrame per batch with tuples in insertion order, with no leak across batches; routes DataFrames to the configured receiver on the data (non-control) channel. - flush: sends a DataFrame with the buffered tuples and resets the buffer when non-empty; is a no-op when called on an empty buffer (so a regression that sends an empty DataFrame breaks here); assigns monotonically increasing sequence numbers across multiple flushes; the same sequence-number stream is shared with the StateFrame on the same channel (pin so a regression that side-channels StateFrame breaks). - sendState: pre-flush bookend drains pending tuples in their own DataFrame BEFORE the StateFrame; pre-flush is a no-op when nothing is pending; trailing post-state flush leaves the buffer clean for the next addTuple. - Edge cases: batchSize=1 flushes after every addTuple; batchSize=0 collapses to the same behavior because the `>=` guard fires for any non-empty buffer (characterized so a future tightening to `>` breaks on purpose). The test wires a real NetworkOutputGateway with a recording handler, so the assertions exercise the production codepath end-to-end (sequence-number assignment, channel-id construction, payload type) rather than mocking the gateway. Closes apache#4957
There was a problem hiding this comment.
Pull request overview
This PR adds focused unit coverage for NetworkOutputBuffer, the batching helper used by partitioners to send DataFrame and StateFrame payloads through NetworkOutputGateway. It fits into Amber’s messaging layer by pinning buffering, flush, and sequencing behavior that previously had no direct spec coverage.
Changes:
- Adds a new
NetworkOutputBufferSpecexercising constructor defaults, tuple buffering, auto-flush, and explicitflush(). - Verifies
sendStateordering and sequence-number behavior using a realNetworkOutputGatewaywith a recording handler. - Adds edge-case coverage for small batch sizes, including
batchSize = 1andbatchSize = 0.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
let's make each test-related PR a bit larger. This is a trade off. In general, we want to keep PRs small so it is easier to review and avoid a large scale revert that conflicts with a lot files. For PRs that adding tests only, as it usually being a new file, less likely to conflict and would be easier to review, we can slightly make the size bigger to reduce the pressure to review a lot of PRs, and reduce the CI pressure. |
Address Copilot review on apache#4958: - Drop the `batchSize = 0` characterization-pin. The workflow-settings UI restricts data-transfer batch size to `>= 1` before the value ever reaches `NetworkOutputBuffer`, so 0 is not a reachable production input. Pinning the current `>=` behavior would enshrine an implementation detail and block a future hardening change that rejects the invalid value at construction time. Replaced the test with a NOTE block documenting the rationale so a future contributor doesn't reflexively re-add it. - Remove unused `ChannelIdentity` import. The amber module enables `-Ywarn-unused:imports` in `amber/build.sbt`, so the unused import was avoidable compiler noise.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4958 +/- ##
=========================================
Coverage 42.22% 42.23%
Complexity 2180 2180
=========================================
Files 980 980
Lines 36287 36287
Branches 3783 3783
=========================================
+ Hits 15321 15324 +3
+ Misses 20038 20034 -4
- Partials 928 929 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Will make future test PRs larger. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot follow-up on apache#4958: my previous NOTE claimed batchSize=0 was not reachable from production because the workflow- settings UI restricts the value to >= 1. That was inaccurate — SyncExecutionResource accepts request.workflowSettings directly from the API and the backend forwards workflowSettings.dataTransferBatchSize into NetworkOutputBuffer with no server-side validation, so a malicious or buggy API caller can reach batchSize=0 today. Replace the misleading NOTE with: - An accurate comment block describing the actual reachability (server-side API path, no validation). - A pendingUntilFixed test asserting the desired hardening: construction with batchSize <= 0 should throw IllegalArgumentException (e.g. via a require(batchSize > 0, ...) guard). Today this test is pending because the constructor accepts non-positive values; once the hardening lands the assertion will pass and pendingUntilFixed will invert that into a deliberate failure forcing the marker to be removed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ngUntilFixed Address Copilot follow-up on apache#4958: pendingUntilFixed alone left the reachable-from-API batchSize<=0 path uncovered by a real regression test. Add a paired characterization test that pins today's lenient behavior (per-tuple flush via the `>=` guard) for both batchSize=0 and batchSize=-1. The pair pattern matches the deserializeKey extra-segments coverage in apache#4954: a future hardening that rejects `<= 0` at construction will break the characterization on purpose AND flip pendingUntilFixed to passing, forcing both markers to be updated together — instead of leaving a regression hole today.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What changes were proposed in this PR?
Adds
NetworkOutputBufferSpeccoveringNetworkOutputBuffer(defined inamber/src/main/scala/org/apache/texera/amber/engine/architecture/sendsemantics/partitioners/Partitioner.scala) — the per-receiver batched-tuple sender every concretePartitionersubclass uses to push data downstream. The class has stateful buffer + flush semantics that nothing currently pins.batchSizedefault =ApplicationConfig.defaultDataTransferBatchSize;to/dataOutputPortexposed as immutable accessors; no implicit auto-flush at construction.addTuplebatchSize; exact-boundary auto-flush; one DataFrame per batch with tuples in insertion order; no cross-batch leakage; data-channel routing to configured receiver.flush()flush()+sendStateshare a sequence streamsendStateaddTuple.batchSizeedge casesbatchSize = 1flushes after everyaddTuple. Reachable-from-APIbatchSize <= 0: server-side path (SyncExecutionResource) acceptsrequest.workflowSettingsdirectly, no>= 1validation, so 0 / negative values reachNetworkOutputBufferfrom a direct API caller despite the UI restricting it. Covered by a paired (characterization +pendingUntilFixed) — characterization pins today's lenient>=-guard per-tuple-flush behavior so a regression in that path surfaces,pendingUntilFixedpins the desired hardening (rejection at construction). When the hardening lands, both flip and must be updated together.The test wires a real
NetworkOutputGatewaywith a recording handler, so assertions exercise the production codepath end-to-end (sequence-number assignment, channel-id construction, payload-type routing) rather than mocking the gateway.No production code changed; this is test-only.
Any related issues, documentation, discussions?
Closes #4957
How was this PR tested?
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.7)