PDGGK opened a new pull request, #9233:
URL: https://github.com/apache/paimon/pull/9233
### Purpose
`ObjectsFile#writeWithoutRolling` reads the stream position inside the
`finally`, ahead of the close:
```java
PositionOutputStream out = fileIO.newOutputStream(path, false);
long pos;
try {
try (FormatWriter writer = writerFactory.create(out, compression)) {
while (records.hasNext()) {
writer.addElement(serializer.toRow(records.next()));
}
}
} finally {
pos = out.getPos(); // <- declared `throws IOException`
out.close(); // <- skipped if it does
}
```
`PositionOutputStream#getPos` is `public abstract long getPos() throws
IOException`. When it throws:
* `out.close()` is never reached and the stream leaks, and
* the `finally`'s exception **replaces** whatever the try body was throwing.
These are not attached as suppressed — that only happens for try-with-resources
— so the original write failure is discarded and the caller sees the `getPos`
error in its place.
The two failures are not independent. The try body usually throws because
the underlying stream is already broken — a full disk, an object store
rejecting the upload — and `getPos` on that same stream is then likely to throw
as well. So the case where the position read fails is largely the same case
where losing the write error hurts most.
### What changes
`pos` is only read by the success-path `return`, so it moves there:
```java
try {
try (FormatWriter writer = writerFactory.create(out, compression)) {
...
}
pos = out.getPos();
} finally {
out.close();
}
return Pair.of(path.getName(), pos);
```
One line moved. On a successful write the value is identical — it is still
read after the `FormatWriter` has been closed and its buffers flushed. On a
failing write, `close()` now runs and the original exception propagates.
### Tests
None added, and I would rather say why than leave it unexplained. Reaching
the branch needs a `FileIO` whose streams fail on `getPos`, and the existing
tests build their manifest files through `table.store()...Factory().create()`,
which supplies the catalog's own `FileIO` with no seam to substitute one.
Building an `ObjectsFile` directly means supplying a serializer, a schema and
reader/writer factories by hand, which is a lot of scaffolding for a one-line
move — but I am happy to write it if you would like the coverage.
`BinaryIndexManifestEntryTest` exercises the success path through this
method and still passes (2 tests, 0 failures). `spotless:check` and
`checkstyle:check` on `paimon-core` are clean.
### API and Format
No change to any public signature, option, or on-disk format. Behaviour
differs only on the failure path, where the stream is now closed and the
original exception is the one that propagates.
### Note
#8927 also touches this method, in the `catch (Throwable e)` block below
(`deleteQuietly` → `deleteQuietlyIgnoringInterrupt`). Different lines and a
different concern, but worth flagging since they are close together.
--
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]
