aglinxinyuan opened a new pull request, #6883:
URL: https://github.com/apache/texera/pull/6883
### What changes were proposed in this PR?
`SyncExecutionResource.collectOperatorResult` read operator results through
an **unbounded** `document.get()` iterator. Three paths abandoned that iterator
with its Iceberg Parquet reader / S3 input stream still open, leaving the GC
finalizer to reclaim it (`[S3InputStream] Unclosed input stream created by ...`
WARNs — the "SyncExecutionResource" rows of the #6881 inventory):
```
document.get() ── opens reader on first next()
├─ visualization branch ── next() once, early return → LEAK
(every sync viz fetch)
├─ oversized-first-tuple branch ── next() once, early return → LEAK
├─ truncation loops ── drain to exhaustion → ok
└─ catch-all on exception ── abandons mid-drain → LEAK
```
Since #6882, a bounded `getRange(from, until)` iterator releases its reader
inside the `next()` that serves its last record, and `hasNext` probes open
nothing. So the fix is to size each read to what the method actually consumes —
no `close()` API added to `VirtualDocument`:
| Path | Consumes | Read after this PR | Close point |
|---|---|---|---|
| visualization result (`totalCount == 1`) | 1 tuple | `getRange(0, 1)` |
inside the single `next()` |
| oversized first tuple | 1 tuple | same `getRange(0, 1)` | inside the
single `next()` |
| table result (truncation loops) | all remaining | `getRange(1,
totalCount)` | inside the `next()` serving the bound / exhaustion |
| swallowed-exception catch-all | partial | catch drains the bounded
remainder | at the bound |
Notes for review:
- The catch-path drain is bounded: it reads at most the records the
successful path would have converted to JSON anyway, and a failing reader ends
the drain early (finalizer remains the backstop for that truly exceptional
case).
- The truncation loops previously drained whatever the iterator produced,
even records appended after `getCount` was sampled; they are now capped at the
same `totalCount` used for the `totalCount`/`skippedRows` fields, keeping the
response internally consistent.
- Deterministic close relies on the iterator semantics of #6882; until that
lands, these bounded reads behave as before (finalizer-reclaimed), so merge
order is safe either way.
### Any related issues, documentation, discussions?
Follow-up to the residual-leak inventory in #6881
(`SyncExecutionResource.collectOperatorResult` rows). Builds on the iterator
restructure in #6882. The remaining #6881 rows (`ResultExportService`
client-disconnect, `InputPortMaterializationReaderThread` interrupt) are lower
priority and left for separate PRs.
### How was this PR tested?
Verified by inspection against the bounded-read semantics of #6882: each
path's read bound equals its consumption, so every path ends in a limit-close,
an exhaustion-close, or nothing opened. The leak itself is only observable as
GC-finalizer WARNs, so no unit test pins it; `getRange` range semantics are
covered by the existing `VirtualDocumentSpec`/`IcebergDocumentSpec` suites, and
this endpoint's behavior is unchanged (same tuples, same truncation, same
response shape). Local `WorkflowExecutionService/compile` and `scalafmtCheck`
pass.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Fable 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]
