rdblue commented on code in PR #9222:
URL: https://github.com/apache/iceberg/pull/9222#discussion_r1416470538
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -188,7 +188,7 @@ protected void cleanUncommitted(Set<ManifestFile>
committed) {
}
}
- this.newManifests = committedNewManifests;
Review Comment:
@jasonf20, I think that this should actually not modify the `newManifests`
field at all and that this is a bug.
When a transaction retries, all of the the new manifests should be reused.
In fact, in the initial version with just one new manifest, it was [left
unchanged](https://github.com/apache/iceberg/pull/6335/files#diff-0ce77e454fbf0eed71c932cb352fb60f405d5ceb636950f3c2af36c6f93a4f48L181-L183).
That way when the operation is re-committed the same set of manifests is
reused. (The transaction is smart enough to only delete the files deleted by
individual operations if the transaction commit succeeds.)
Setting this to `null` would fix the problem by causing Iceberg to rewrite
the manifests every time the transaction is attempted. That works, but the
extra writes are unnecessary.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
