JandyTenedora commented on code in PR #16960:
URL: https://github.com/apache/iceberg/pull/16960#discussion_r3693258002
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -780,24 +781,22 @@ private static long rewriteDVFile(
String sourcePrefix,
String targetPrefix)
throws IOException {
- List<Blob> rewrittenBlobs = Lists.newArrayList();
- try (PuffinReader reader =
Puffin.read(io.newInputFile(deleteFile.location())).build()) {
- // Read all blobs and rewrite them with updated referenced data file
paths
- for (Pair<org.apache.iceberg.puffin.BlobMetadata, ByteBuffer> blobPair :
+ try (PuffinReader reader =
Puffin.read(io.newInputFile(deleteFile.location())).build();
+ PuffinWriter writer =
+
Puffin.write(outputFile).createdBy(IcebergBuild.fullVersion()).build()) {
Review Comment:
Hey @szehon-ho thanks so much for the feedback! If I understand correctly,
prior to this change this wasn't an issue since the read and write boundaries
were definitively separated. All source reads completed before the writer was
ever opened, so a read failure meant no output file existed and retries worked
cleanly.
I've separated the reader and writer creation into different try statements
so that a corrupt footer failure no longer creates the staging file. But even
then, readAll [loads blob payloads
lazily](https://github.com/apache/iceberg/blob/25cc463c0d2108db6a9427b7f8e2b191f7913061/core/src/main/java/org/apache/iceberg/puffin/PuffinReader.java#L130-L148),
so a blob read failure can still happen after the writer has already been
created. I'm worried this will hit the same `AlreadyExistsException` you
mentioned.
To handle that, I've added a best effort cleanup: source read failures throw
a RuntimeException, which gets caught in the outer catch block where I delete
the partial output file before rethrowing.
Let me know your thoughts on this approach, whether the reader/writer
separation (which I believe alone addresses this comment's concern) is
sufficient, or if there's something I'm missing.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
