aokolnychyi commented on a change in pull request #2328:
URL: https://github.com/apache/iceberg/pull/2328#discussion_r592944422
##########
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 am afraid this will be dangerous. This means we have to update all
places that throw a specific exception like `AlreadyExistsException` to now
throw `CommitFailedException`. This is error-prone and we potentially lose
helpful details. We could throw `CommitFailedException` and add the specific
exception as a cause but the first point still holds.
Also, an error log message will most likely be ignored by the user. This is
a case where we really want to propagate as much info as possible.
Would it make sense to introduce a new exception type? E.g.,
`UnknownCommitStateException`? That way, we can keep the existing logic for
handling exceptions except cases where we really don't know what happens.
----------------------------------------------------------------
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]
