rdblue commented on a change in pull request #2328:
URL: https://github.com/apache/iceberg/pull/2328#discussion_r595593348
##########
File path: core/src/main/java/org/apache/iceberg/SnapshotProducer.java
##########
@@ -293,8 +293,15 @@ public void commit() {
taskOps.commit(base, updated.withUUID());
});
+ } catch (CommitFailedException commitFailedException) {
+ // We have an acknowledged failure from the Catalog. We are confident
that the commit has not been applied
+ Exceptions.suppressAndThrow(commitFailedException, this::cleanAll);
} catch (RuntimeException e) {
- Exceptions.suppressAndThrow(e, this::cleanAll);
+ LOG.error("Cannot determine whether the commit was successful or not,
the underlying data files may or " +
Review comment:
I think there are a couple of issues here that we should address
separately.
First, I agree that it is a good idea to add a way to signal that a commit
is idempotent. Some operations are already idempotent, like file rewrites
because we validate that all of the rewritten files are still part of the
table. If we update paths that guarantee idempotent commits to signal this and
handle `UnknownCommitStateException`, then we really reduce the incidence of
the problem.
Second, I think we still need to agree on the default behavior. While I
really don't like the idea of allowing a retry that will write duplicate data,
I think that this has convinced me that silently duplicating data is a better
outcome.
In our environment, we use versioned buckets so we can always un-delete
metadata files that are missing, but that's not always the case. If those files
are actually gone, then it is much worse because you have missing data and
don't know which data files were in the missing commit without a lot of work. I
think this problem is worse than the duplicate data.
A second compelling argument for changing the default is that deleting the
metadata files leaks the problem to other writers. All concurrent processes are
blocked if a table is broken, rather than blocking just a single writer.
In the end, I think that the right thing is to not delete the files and to
throw `UnknownCommitStateException` as proposed. That handles interactive cases
and also makes it so schedulers can handle a job failure by blocking just a
single workflow and not all workflows operating on a table. And idempotent jobs
should not be affected.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
