Missing parquet-file due to wrong state in writer

[fpetersen-gl]

Apache Iceberg version

1.9.1 (latest release)

Query engine

None

Please describe the bug 🐞

Description of the problem

If the connection to the underlying IO (we're using S3) is cut in the wrong moment, a parquet-file cannot be written correctly, but its writer assumes it was.
This writer is in our case wrapped by a FanoutWriter, which happily returns this file as result after closing all of them, making it appear in the metadata.
The next call to close all writers being part of the FanoutWriter ignores the failed writer, as it is already marked as closed.

Result: A snapshot is being written which references the missing parquet-file.

Code analysis

Setting the internal state to closed in ParquetWriter.close() happens already before any code is executed that could potentially break. There's writer.end(metadata), which can throw an IOException, but flushRowGroup(true) before can throw an UncheckedIOException, thus leaving the writer in a wrong state.

Possible solution

First naive thing that comes to my mind would be to move this.closed = true to the very end of the method. This would only change the state iff all procedures for closing the writer have been executed successfully.
I'll try to come up with a test to reproduce this issue, will update the ticket afterwards.

Willingness to contribute

• I can contribute a fix for this bug independently
• I would be willing to contribute a fix for this bug with guidance from the Iceberg community
• I cannot contribute a fix for this bug at this time

[tellistone]

This is a deceptively severe problem that typically results in data loss in workflows it effects, hoping this can get turned around quickly.

[nastra]

@stevenzwu I've added this to 1.10 and I'll be investigating the issue later today.

[tellistone]

Thanks for taking a look so quickly!

May I ask what the release timeline is like for 1.10?

(I will commence praying to the data management gods that it occurs within the next 10 weeks before our next release)

[nastra]

1.10 should be released within the next few weeks but as I mentioned #13565 (comment) I think the issue would first need to be fixed in Parquet before we could change the closing behavior in ParquetWriter

[stevenzwu]

If this requires fix in upstream Parquet first, should we remove this as a blocker for the 1.10.0 release? I am hoping that we can release 1.10.0 in the next 1-2 weeks. It has been delayed for quite some time already and blocking the new features like Spark 4.0 and Flink 2.0 support. If the turn-around can be quick, I am more than happy to include this in 1.10.0 release.

This can also be included in follow-up patch release for 1.10.x.

[tellistone]

We've taken a look at the blocker apache/parquet-java#3254

Unfortunately a bit beyond our current understanding of parquet's internals to fix.

[kroepke]

Hi all!

I've had another look at this with @fpetersen-gl today. First, let me give you a brief description of how this played out in our code, and then suggest a few possible ways to fix/mitigate it.

In our code, we regularly flush a map of tables using fan-out writers from a single thread. We do that in a simple for loop with an exception handler for IOException (point 1).
We run channel.writer.close(); which can fail in the manner described above (channel is our wrapper around fan-out writers).
Unfortunately, a lot of code also throws UncheckedIOException, which our code calling Closeable#close() didn't expect since it already has a signature with the checked IOException, thus it missed handling it.
That part is a bug in our code (point 2), leading to an inconsistent state of its channel map, with some of them closed already, the one that threw the exception marked as closed (point 3), and the others still being writable.
At the next flush interval in our code, we'd use the previous channel map and run close() on each fan-out writer again.
Let's suppose the intermittent network IO that cause the exception is fixed now, all close() calls will successfully return, even the ones that are already closed in the first loop.

Long story short, I believe that Iceberg's API doesn't expect anyone to run close() more than once, and due to the ordering of setting the closed flag like @fpetersen-gl described, it will silently return even if it has encountered an error from its underlying writer.

I think our bug, point 2, would've been much easier to spot if point 3 would throw an IllegalStateException or something like (e.g. java.util.Stream does this when you interact with a closed instance) when it is being used after close.
Silently continuing masks important errors, and in our case, leads to data loss. I couldn't find anything in the javadocs or anywhere else that makes it clear that re-closing something will lead to bad outcomes, imho that should be fixed.
In particular, it would be helpful if these codepaths would throw IllegalStateExceptions to make it less likely to misuse the library.

I'm also unclear as to what the point of throwing unchecked exceptions from methods that declare the checked version of it is, it makes it very hard to reason about, I think that should be fixed as well.

Looking forward to opinions on this!