delete file by mistake when rewriting iceberg table throw CommitStateUnknownException #2932

wg1026688210 · 2021-08-04T11:18:02Z

This pr is to solve the problem of deleting files by mistake in rewriting table if we can’t judge whether the submission is successful when commiting.
The solution is that we only print some logs and do not clean up the files if throw CommitStateUnknownException when commiting ,leave the cleanup work to RemoveOrphanFiles

…UnknownException

core/src/main/java/org/apache/iceberg/actions/BaseRewriteDataFilesAction.java

stevenzwu

left one minor comment

wg1026688210 · 2021-08-06T02:34:29Z

cc @stevenzwu @openinx @rdblue

left one minor comment

thanks for your concern, it's ready for review

rdblue · 2021-08-06T15:25:26Z

core/src/main/java/org/apache/iceberg/actions/BaseRewriteDataFilesAction.java

    return new RewriteDataFilesActionResult(currentDataFiles, addedDataFiles);
  }

-


Please remove unnecessary changes like this and reformatting lines that don't need to change. We avoid changes that aren't required to avoid git commit conflicts.

rdblue · 2021-08-06T15:28:19Z

This looks valid to me. The action will clean up data files for any exception, but we should not do that when the commit state is unknown. FYI @RussellSpitzer.

@wg1026688210, can you remove any unnecessary formatting and whitespace changes? It would also be great to update the description with the case that this fixes. Thanks for working on this!

RussellSpitzer · 2021-08-06T15:32:31Z

Yeah this is definitely the right thing to do, we are handling this in the new code (

iceberg/core/src/main/java/org/apache/iceberg/actions/RewriteDataFilesCommitManager.java

Lines 97 to 109 in a1bd63d

    
           public void commitOrClean(Set<RewriteFileGroup> rewriteGroups) { 
        
             try { 
        
               commitFileGroups(rewriteGroups); 
        
             } catch (CommitStateUnknownException e) { 
        
               LOG.error("Commit state unknown for {}, cannot clean up files because they may have been committed successfully.", 
        
                   rewriteGroups, e); 
        
               throw e; 
        
             } catch (Exception e) { 
        
               LOG.error("Cannot commit groups {}, attempting to clean up written files", rewriteGroups, e); 
        
               rewriteGroups.forEach(this::abortFileGroup); 
        
               throw e; 
        
             } 
        
           }

) as well and while this path is deprecated now I think we should definitely fix it to avoid causing issues while the path is still available.

wg1026688210 · 2021-08-07T03:55:37Z

This looks valid to me. The action will clean up data files for any exception, but we should not do that when the commit state is unknown. FYI @RussellSpitzer.

@wg1026688210, can you remove any unnecessary formatting and whitespace changes? It would also be great to update the description with the case that this fixes. Thanks for working on this!

ok

wg1026688210 · 2021-08-07T03:56:23Z

Yeah this is definitely the right thing to do, we are handling this in the new code (

iceberg/core/src/main/java/org/apache/iceberg/actions/RewriteDataFilesCommitManager.java

Lines 97 to 109 in a1bd63d

public void commitOrClean(Set<RewriteFileGroup> rewriteGroups) {

try {

commitFileGroups(rewriteGroups);

} catch (CommitStateUnknownException e) {

LOG.error("Commit state unknown for {}, cannot clean up files because they may have been committed successfully.",

rewriteGroups, e);

throw e;

} catch (Exception e) {

LOG.error("Cannot commit groups {}, attempting to clean up written files", rewriteGroups, e);

rewriteGroups.forEach(this::abortFileGroup);

throw e;

}

}

) as well and while this path is deprecated now I think we should definitely fix it to avoid causing issues while the path is still available.

looking forward to using the new version rewrite

rdblue · 2021-08-09T15:56:09Z

core/src/main/java/org/apache/iceberg/actions/BaseRewriteDataFilesAction.java

-                                long startingSnapshotId) {
+  private void replaceDataFiles(
+      Iterable<DataFile> deletedDataFiles, Iterable<DataFile> addedDataFiles,
+      long startingSnapshotId) {


@wg1026688210, please remove changes like this that are unnecessary.

rdblue · 2021-08-09T15:58:15Z

core/src/main/java/org/apache/iceberg/actions/BaseRewriteDataFilesAction.java

-          .onFailure((location, exc) -> LOG.warn("Failed to delete: {}", location, exc))
-          .run(fileIO::deleteFile);
+      if (e instanceof CommitStateUnknownException) {
+        LOG.warn("for unknown commiting state ,we should not delete file ");


Please copy the other warning messages here. This needs to conform more to the error message expectations that we use:

Messages should use sentence case

Messages should not end in whitespace

Do not use personal pronouns

Be clear about what happened and what action is being taken

This should be something more like "Skipping commit cleanup: commit state is unknown". But check the other places where we catch CommitStateUnknownException and use a similar message.

thanks, learned a lot for me

rdblue · 2021-08-09T15:58:43Z

core/src/main/java/org/apache/iceberg/actions/BaseRewriteDataFilesAction.java

-          .suppressFailureWhenFinished()
-          .onFailure((location, exc) -> LOG.warn("Failed to delete: {}", location, exc))
-          .run(fileIO::deleteFile);
+      if (e instanceof CommitStateUnknownException) {


Why is this not a separate catch block? I think it should be a catch instead of using instanceof.

2.Remove redundant code format

rdblue · 2021-08-10T15:06:32Z

core/src/main/java/org/apache/iceberg/actions/BaseRewriteDataFilesAction.java

+      doReplace(deletedDataFiles, addedDataFiles, startingSnapshotId);
+    } catch (CommitStateUnknownException e) {
+      LOG.warn("Commit state unknown for files: {}, cannot clean up files because they may have been committed " +
+          "successfully.", Lists.newArrayList(addedDataFiles), e);


Let's remove the list of data files from logs. That is going to be too long. Instead, let's use "Commit state unknown, cannot clean up files that may have been committed"

rdblue · 2021-08-10T15:07:18Z

core/src/main/java/org/apache/iceberg/actions/BaseRewriteDataFilesAction.java

+          "successfully.", Lists.newArrayList(addedDataFiles), e);
+      throw e;
    } catch (Exception e) {
+      LOG.warn("Failed to rewrite, files {} will be delete", Lists.newArrayList(addedDataFiles));


Let's remove the file list here as well. Instead, use "Failed to commit rewrite, cleaning up rewritten files"

wg1026688210 · 2021-11-01T04:52:15Z

Hi,@rdblue @RussellSpitzer .the pr appears conflicts. Do we need to maintain the rewrite api of old version next release. If necessary, I will resolve this conflict.

rdblue · 2021-11-01T23:50:36Z

I'll defer to @RussellSpitzer on whether we should fix the old API.

RussellSpitzer · 2021-11-17T22:19:09Z

Sorry for taking so long to get to this, I think this would be worth keeping just because of how difficult it is to deal with this situation. Please update the PR and I will merge it in.

…elete_file_by_mistake � Conflicts: � spark/src/test/java/org/apache/iceberg/actions/TestRewriteDataFilesAction.java

wg1026688210 · 2021-12-04T11:50:48Z

Ok, I will resolve this conflict today

wg1026688210 · 2021-12-04T12:45:22Z

Sorry for taking so long to get to this, I think this would be worth keeping just because of how difficult it is to deal with this situation. Please update the PR and I will merge it in.

hi~ @RussellSpitzer the conflict has been resolved

RussellSpitzer

Thanks for the fix!

delete file by mistake when rewriting iceberg table throw CommitState…

286bcd3

…UnknownException

github-actions bot added the core label Aug 4, 2021

stevenzwu reviewed Aug 5, 2021

View reviewed changes

core/src/main/java/org/apache/iceberg/actions/BaseRewriteDataFilesAction.java Show resolved Hide resolved

stevenzwu approved these changes Aug 5, 2021

View reviewed changes

add unit test

c6c1e11

github-actions bot added the spark label Aug 5, 2021

wg1026688210 added 5 commits August 5, 2021 21:09

checkstyle

2610653

checkstyle

45f70cf

checkstyle

7ad1174

checkstyle

31e1674

remove empty method

5ab67b5

wg1026688210 changed the title ~~delete file by mistake when rewriting iceberg table when replaceDataFiles throw CommitStateUnknownException~~ delete file by mistake when rewriting iceberg table throw CommitStateUnknownException Aug 5, 2021

wg1026688210 added 3 commits August 6, 2021 13:23

mock rewrite action

48ba93c

checkstyle

a1a8411

optimize code

0712815

rdblue reviewed Aug 6, 2021

View reviewed changes

remove unnecessary changes

88ba568

wg1026688210 closed this Aug 9, 2021

wg1026688210 reopened this Aug 9, 2021

wg1026688210 closed this Aug 9, 2021

wg1026688210 reopened this Aug 9, 2021

rdblue reviewed Aug 9, 2021

View reviewed changes

1.fix bad exception message

32ad0af

2.Remove redundant code format

Remove redundant code format

b718149

rdblue reviewed Aug 10, 2021

View reviewed changes

wg1026688210 added 2 commits August 11, 2021 10:37

fix bad exception message

28ae92d

fix bad exception message

5db65fe

rdblue approved these changes Aug 11, 2021

View reviewed changes

Merge branch 'master' of https://github.com/apache/iceberg into fix/d…

721ee26

…elete_file_by_mistake � Conflicts: � spark/src/test/java/org/apache/iceberg/actions/TestRewriteDataFilesAction.java

RussellSpitzer approved these changes Dec 4, 2021

View reviewed changes

RussellSpitzer merged commit c6e68d8 into apache:master Dec 4, 2021

		return new RewriteDataFilesActionResult(currentDataFiles, addedDataFiles);
		}

delete file by mistake when rewriting iceberg table throw CommitStateUnknownException #2932

delete file by mistake when rewriting iceberg table throw CommitStateUnknownException #2932

Uh oh!

Conversation

wg1026688210 commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

stevenzwu left a comment

Choose a reason for hiding this comment

Uh oh!

wg1026688210 commented Aug 6, 2021

Uh oh!

rdblue Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

rdblue commented Aug 6, 2021

Uh oh!

RussellSpitzer commented Aug 6, 2021

Uh oh!

wg1026688210 commented Aug 7, 2021

Uh oh!

wg1026688210 commented Aug 7, 2021

Uh oh!

rdblue Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

rdblue Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wg1026688210 Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

rdblue Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

rdblue Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

rdblue Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

wg1026688210 commented Nov 1, 2021

Uh oh!

rdblue commented Nov 1, 2021

Uh oh!

RussellSpitzer commented Nov 17, 2021

Uh oh!

wg1026688210 commented Dec 4, 2021

Uh oh!

wg1026688210 commented Dec 4, 2021

Uh oh!

RussellSpitzer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wg1026688210 commented Aug 4, 2021 •

edited

Loading

rdblue Aug 9, 2021 •

edited

Loading