davidzollo commented on PR #11929: URL: https://github.com/apache/seatunnel/pull/11929#issuecomment-5391951756
@DanielLeens thanks for the thorough review — all findings are addressed on head `bdc10231899a`. **Issue 1 (unchecked close failures bypass aggregation)** — fixed in `4cea5d7a015`. `close()` now routes `RuntimeException` from `arrowStreamReader.close()` into the same `closeException` aggregation as `IOException`, so an allocator failure in the `finally` is always attached via `addSuppressed` instead of being silently dropped. A new Mockito test (`testCloseAggregatesUncheckedReaderFailureWithAllocatorFailure`) pins the contract: unchecked reader failure is the primary, allocator failure is suppressed, order still reader-then-allocator. **Issue 2 (Mockito-only tests cannot catch the real regression)** — fixed in the same commit with two real-allocator round-trip tests through the production `byte[]` constructor: - `testReadArrowDrainsRealStreamAndClosesWithoutLeak`: plain int column, full drain, data asserted. - `testReadArrowReleasesDictionaryAllocationsOnClose`: a dictionary-encoded VarChar column (written via `DictionaryProvider` + `DictionaryEncoder`). This is the faithful #9863 regression: dictionary batches are allocated by `ArrowStreamReader` outside the `VectorSchemaRoot`, so the pre-fix order (`rootAllocator.close()` before reader close) fails with "Memory was leaked by query" even for a fully drained stream. I verified against Arrow 15.0.1 that a fully drained, dictionary-free stream does not reproduce the leak under the old order (root.close() already returns all root buffers), so the dictionary variant is the test that actually discriminates old vs new behavior. The encoded column is intentionally absent from the SeaTunnel schema to keep the assertion on resource release via the unknown-field skip path. **Non-blocking notes** — both taken: the test constructor is now `@VisibleForTesting`, and it assigns the caller-supplied reader/allocator before `initFieldIndexMap(...)`, so a throwing schema can no longer strand the resources. **Duplicate consolidation** — #10958 is closed as superseded by this PR (same functional commits, Apache-owned branch, review findings addressed here). #11593 stays open for maintainer decision; with the round-trip tests added here the coverage gap relative to it is closed. **CI status** — the previous run failed for two reasons unrelated to this diff: `updated-modules-integration-test-part-5` deterministically exceeded its 180-minute budget on both JDKs in both attempts (measured: clickhouse 60m + cdc-mongodb 27m + HbaseIT 85m+ still running at cancellation — a `connector-common` change fans out to the full e2e matrix), and part-4 hit the known Couchbase container auth flake (passed in attempt 1, failed in attempt 2). The branch is now updated to current dev and carries the uniform 300-minute timeout fix, also proposed standalone as #11956; whichever merges first makes the other a no-op. -- 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]
