[GitHub] [iceberg] rdblue commented on a change in pull request #2328: (#2317) Stop removal of files when catalog state is uncertain - HiveCatalog

[GitBox <gi...@apache.org>]

rdblue commented on a change in pull request #2328:
URL: https://github.com/apache/iceberg/pull/2328#discussion_r594597450



##########
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:
       It's very dangerous to change the logic here. If there is an exception where it is unknown whether or not the table reference was updated, then it is safer to drop the files and assume that the commit was not successful. That's because we still need to propagate the exception, which signals that the commit failed. Most of the time, that will cause a higher-level retry of the whole operation, like re-running a scheduled job with the same data. If that happens and both commits are actually successful, then we have a situation where incoming records were silently duplicated.
   
   I like the idea of throwing `UnknownCommitStateException`, but I think we would need some way to ensure that it is actually handled. An idempotent committer (like the Flink sink) could simply swallow the exception and retry. But without coordination between the `TableOperations` that decides whether to throw this exception and the committer I think there is still a problem. Could we add an option to `SnapshotProducer` to signal whether it would be safe and then skip cleanup if the flag is set?




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org