aglinxinyuan opened a new issue, #8402:
URL: https://github.com/apache/texera/issues/8402
### Task Summary
`FlowControlSpec` contains a test whose name claims a guarantee nothing in
the file pins:
```scala
"FlowControl" should "trip the size-cap assertion for a message that exceeds
maxByteAllowed" in {
// ...
val fc = new FlowControl()
(1L to 1000L).foreach(i => fc.getMessagesToSend(msg(i)))
succeed
}
```
It ends in a bare `succeed` and checks no property of the result. Its own
comment concedes it cannot synthesize an oversized payload, then claims to
"lock down the guard shape" by sending 1000 messages — but nothing about those
1000 calls is asserted.
The guard it is named after is `assert(creditNeeded <= maxByteAllowed, ...)`
at the top of `FlowControl.getMessagesToSend`. Deleting that block from the
production file leaves all 14 tests in `FlowControlSpec` green. The fixture
cannot reach it: `FixedSizePayload` reports 200 bytes against a configured cap
of 1,600,000,000, and 1000 x 200 = 200,000 bytes does not exhaust credit
either, so the messages never take the stashing path.
The guard is reachable from a unit test, contrary to what the test's comment
asserts. `DataFrame.inMemSize` is `frame.map(_.inMemSize).sum`, which does not
deduplicate, so an `Array[Tuple]` holding the same tuple reference N times
reports N times its size — a payload over a multi-GB cap costs a few hundred KB
of real memory. No production seam is needed.
Proposed cleanup, test-only:
- add a test that actually trips the guard with such a payload, asserting
the `AssertionError` and that the rejection leaves the channel untouched
(credit unchanged, not marked overloaded — distinguishing the guard from the
out-of-credit branch below it);
- replace the assertion-free body with one that pins the fast path the guard
sits on: what `getMessagesToSend` returns per under-cap message and the
resulting `getCredit` / `isOverloaded` state across a batch, which is what
catches accounting that is right for the first message and then drifts;
- derive expected values from the fixture (`msgSize` from
`WorkflowMessage.getInMemSize`, `maxBytes` from `ApplicationConfig`) rather
than hard-coding them, plus a precondition that the batch fits under the cap;
- rewrite the stale comment.
No production change and no reformatting; one test file. Two unrelated
weaknesses in neighbouring tests in the same file — a tautological `assert(seen
== stashed.size)` in the multi-run drain test, and a suite-constructor
`assert(msgSize == 200L)` that would abort the suite rather than fail a test —
should be handled separately, not folded into this cleanup.
### Task Type
- [x] Refactor / Cleanup
### Was this issue authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
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]
