PDGGK commented on PR #9233:
URL: https://github.com/apache/paimon/pull/9233#issuecomment-5302204530
You're right, and the claim in my description was wrong. A `finally` that
throws discards the exception in flight, so `out.close()` failing would still
have replaced the write failure — the very thing the PR said it fixed. Pushed
`a876331` using try-with-resources as you suggest.
```java
long pos;
try (PositionOutputStream out = fileIO.newOutputStream(path, false)) {
try (FormatWriter writer = writerFactory.create(out, compression)) {
while (records.hasNext()) {
writer.addElement(serializer.toRow(records.next()));
}
}
pos = out.getPos();
}
```
I kept the two blocks nested rather than folding them into one resource
list, because the position has to be read after the writer has flushed and
before the stream closes; a single list would close the writer only on the way
out, after `getPos()`.
I also added a test, since the thing I got wrong was exactly the exception
plumbing and I would rather it be pinned than argued. It drives a stub whose
writer close and stream close both throw:
```
Expecting message to be:
"writer close failed"
but was:
"stream close failed"
```
That is the old `try`/`finally` under the new test — your case, reproduced.
With `a876331` the cause is `writer close failed` and `stream close failed` is
attached to it as suppressed. A second case covers the success path, asserting
the stream is still closed and the position is the one returned.
`ObjectsFileWriteFailureTest` and `BinaryIndexManifestEntryTest`: 4 tests, 0
failures. `spotless:apply` and `checkstyle:check` on `paimon-core` are clean.
Thanks for catching it — and for #9227.
--
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]
