DanielLeens commented on PR #11593:
URL: https://github.com/apache/seatunnel/pull/11593#issuecomment-5412498540
Thanks for the follow-up, but I need to flag a discrepancy: this comment
describes the code as if it were still on the pre-fix version (single shared
`try` block, `catch (IOException e)` only, no per-resource `finally`), and
re-lists Issues 1-7 as open with the old line numbers (`:322-332`, `:333`,
etc.). That is not what's in the tree at this head.
I just re-pulled `159143ae117b` into a clean worktree and read
`ArrowToSeatunnelRowReader.java` directly (not from any prior write-up). The
actual current `close()` is:
```java
@Override
public void close() {
Exception closeException = null;
try {
closeException = closeResource(arrowStreamReader, closeException);
} finally {
arrowStreamReader = null;
root = null;
fieldVectors = null;
}
try {
closeException = closeResource(rootAllocator, closeException);
} finally {
rootAllocator = null;
}
if (closeException != null) {
throw new RuntimeException("failed to close arrow reader
resources.", closeException);
}
}
private Exception closeResource(AutoCloseable resource, Exception
closeException) {
if (resource == null) {
return closeException;
}
try {
resource.close();
} catch (Exception e) {
if (closeException == null) {
return e;
}
closeException.addSuppressed(e);
}
return closeException;
}
```
This is the "[Fix][Connector-V2] Make Arrow reader close exception-safe"
commit already on this head (also visible in the commit history, and matching
what I confirmed line-by-line in my 2026-08-23 11:26 UTC review):
`arrowStreamReader` and `rootAllocator` are now closed in independent
`try/finally` blocks, so a failure in the reader close no longer skips the
allocator close (former Issue 1) — `catch (Exception e)` in `closeResource`
also covers the unchecked `IllegalStateException` case (former Issue 3).
`fieldVectors = null` is set alongside `root = null` (former Issue 5), the
`byteArrayInputStream` field is gone (former Issue 6), and the per-batch log is
`log.debug` rather than deleted (former Issues 4/7). On the test side,
`testReadArrowClosesOwnedResources` now uses try-with-resources and a new
`testCloseClosesAllocatorWhenArrowReaderCloseFails` test (both present at lines
429 and 446 of the current test file) mocks the reader-close failure and
asserts the allocator is stil
l closed — directly exercising the failure path (former Issue 2).
So all 7 previously-raised issues are resolved in the code actually on this
head, not just claimed to be. The only two open items from my side are the two
new Low-severity, non-blocking notes from my 2026-08-23 review (a minor
test-hygiene nit on the new mock test, and an optional improvement to name the
failing resource in the wrapped exception message) — neither blocks merge.
The real remaining blockers are procedural, same as I said on 2026-08-23:
this PR is still `draft`, so `Build`/UT CI hasn't actually run on this head yet
(only `labeler` has completed), and there's a duplicate-effort question with
#10958 and #11929 which touch the same file for the same underlying bug (#9863)
— a maintainer should pick one implementation. Once this is marked ready for
review and the full CI (including the two Arrow-close tests) comes back green,
this looks mergeable from the code side.
--
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]
