You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/05/21 10:36:05 UTC
[GitHub] [iceberg] coolderli opened a new pull request #2621: Core: Skip delete files when commit-status is unknown.
coolderli opened a new pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621
In #2317 when commit-status is unknown, we will not delete files. But when commit status is known when we rewrite files, it will delete data files even when the commit status is unknown. So I think we should skip deleting files when the commit status is unknown.
--
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
[GitHub] [iceberg] coolderli commented on a change in pull request #2621: Core: Skip delete files when commit-status is unknown.
Posted by GitBox <gi...@apache.org>.
coolderli commented on a change in pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621#discussion_r637380556
##########
File path: core/src/main/java/org/apache/iceberg/actions/BaseRewriteDataFilesAction.java
##########
@@ -267,7 +268,10 @@ private void replaceDataFiles(Iterable<DataFile> deletedDataFiles, Iterable<Data
RewriteFiles rewriteFiles = table.newRewrite();
rewriteFiles.rewriteFiles(Sets.newHashSet(deletedDataFiles), Sets.newHashSet(addedDataFiles));
commit(rewriteFiles);
- } catch (Exception e) {
+ } catch (CommitStateUnknownException e) {
+ LOG.warn("Skip delete files because we can not determine whether the commit was successful or not.");
Review comment:
done
--
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
[GitHub] [iceberg] RussellSpitzer commented on pull request #2621: Core: Skip delete files when commit-status is unknown.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621#issuecomment-845937255
That makes sense to me, could you add a test to verify?
--
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
[GitHub] [iceberg] coolderli commented on pull request #2621: Core: Skip delete files when commit-status is unknown.
Posted by GitBox <gi...@apache.org>.
coolderli commented on pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621#issuecomment-845857371
@RussellSpitzer @rdblue What do you think of this
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #2621: Core: Skip delete files when commit-status is unknown.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621#discussion_r637228203
##########
File path: core/src/main/java/org/apache/iceberg/actions/BaseRewriteDataFilesAction.java
##########
@@ -267,7 +268,10 @@ private void replaceDataFiles(Iterable<DataFile> deletedDataFiles, Iterable<Data
RewriteFiles rewriteFiles = table.newRewrite();
rewriteFiles.rewriteFiles(Sets.newHashSet(deletedDataFiles), Sets.newHashSet(addedDataFiles));
commit(rewriteFiles);
- } catch (Exception e) {
+ } catch (CommitStateUnknownException e) {
+ LOG.warn("Skip delete files because we can not determine whether the commit was successful or not.");
Review comment:
I think this error message could be a little more clear. We try to write these in a form like "Cause: result". In this case, I'd recommend "Failed to check commit status: skipping data file deletion".
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #2621: Core: Skip delete data files when commit-state is unknown.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621#discussion_r637606872
##########
File path: core/src/main/java/org/apache/iceberg/actions/BaseRewriteDataFilesAction.java
##########
@@ -262,12 +264,16 @@ public RewriteDataFilesActionResult execute() {
return tasksGroupedByPartition.asMap();
}
- private void replaceDataFiles(Iterable<DataFile> deletedDataFiles, Iterable<DataFile> addedDataFiles) {
+ @VisibleForTesting
+ void replaceDataFiles(Iterable<DataFile> deletedDataFiles, Iterable<DataFile> addedDataFiles) {
try {
RewriteFiles rewriteFiles = table.newRewrite();
rewriteFiles.rewriteFiles(Sets.newHashSet(deletedDataFiles), Sets.newHashSet(addedDataFiles));
commit(rewriteFiles);
- } catch (Exception e) {
+ } catch (CommitStateUnknownException e) {
+ LOG.warn("Failed to check commit status: skipping data file deletion.", e);
+ throw e;
+ } catch (Exception e) {
Review comment:
Looks like formatting is incorrect here: there are two spaces between `}` and `catch` for both clauses and that is causing git to detect that the catch statement for `Exception` has changed when it actually hasn't. Can you fix those, please? We try to minimize changes that can cause git conflicts.
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #2621: Core: Skip delete data files when commit-state is unknown.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621#discussion_r637607406
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBaseRewriteDataFilesAction.java
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.actions;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.CombinedScanTask;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileScanTask;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.TableTestBase;
+import org.apache.iceberg.exceptions.CommitStateUnknownException;
+import org.apache.iceberg.io.FileIO;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.mockito.internal.util.collections.Sets;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
+
+@RunWith(Parameterized.class)
+public class TestBaseRewriteDataFilesAction extends TableTestBase {
+ @Parameterized.Parameters(name = "formatVersion = {0}")
+ public static Object[] parameters() {
+ return new Object[] {1, 2};
+ }
+
+ public TestBaseRewriteDataFilesAction(int formatVersion) {
+ super(formatVersion);
+ }
+
+ @Test
+ public void testReplaceCommitSuccess() throws IOException {
+ TestRewriteDataFileAction testRewriteDataFileAction = new TestRewriteDataFileAction(table);
+ DataFile dataFile = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withRecordCount(10)
+ .withFileSizeInBytes(10)
+ .build();
+
+ DataFile rewrite = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withRecordCount(10)
+ .withFileSizeInBytes(10)
+ .build();
+
+ table.newFastAppend()
+ .appendFile(dataFile)
+ .commit();
+
+ table.newFastAppend()
+ .appendFile(rewrite)
+ .commit();
+
+ testRewriteDataFileAction.replaceDataFiles(Sets.newSet(dataFile), Sets.newSet(rewrite));
+
+ Assert.assertTrue(new File(dataFile.path().toString()).exists());
+ Assert.assertTrue(new File(rewrite.path().toString()).exists());
+ }
+
+ @Test
+ public void testReplaceCommitStateUnknown() throws IOException {
+ TestRewriteDataFileAction testRewriteDataFileAction = new TestRewriteDataFileAction(table);
+ DataFile dataFile = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withFileSizeInBytes(10)
+ .withRecordCount(10)
+ .build();
+
+ DataFile rewrite = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withFileSizeInBytes(10)
+ .withRecordCount(10)
+ .build();
+
+ table.newFastAppend()
+ .appendFile(dataFile)
+ .commit();
+ table.newFastAppend()
+ .appendFile(rewrite)
+ .commit();
+
+ BaseRewriteDataFilesAction spyRewriteDataFileAction = spy(testRewriteDataFileAction);
+ doThrow(new CommitStateUnknownException(new Exception("Commit-state is unknown.")))
+ .when(spyRewriteDataFileAction)
+ .commit(any());
Review comment:
I don't think that this actually tests the case that you're trying to. This throws an exception instead of calling `replaceDataFiles`. In order to test the catch clause you added, you'd need the table's rewrite action to throw the `CommitStateUnknownException`. You could do that with table spy that returns a special rewrite that throws the exception from `commit` after calling `super.commit()` successfully.
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #2621: Core: Skip delete data files when commit-state is unknown.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621#discussion_r637607406
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBaseRewriteDataFilesAction.java
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.actions;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.CombinedScanTask;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileScanTask;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.TableTestBase;
+import org.apache.iceberg.exceptions.CommitStateUnknownException;
+import org.apache.iceberg.io.FileIO;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.mockito.internal.util.collections.Sets;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
+
+@RunWith(Parameterized.class)
+public class TestBaseRewriteDataFilesAction extends TableTestBase {
+ @Parameterized.Parameters(name = "formatVersion = {0}")
+ public static Object[] parameters() {
+ return new Object[] {1, 2};
+ }
+
+ public TestBaseRewriteDataFilesAction(int formatVersion) {
+ super(formatVersion);
+ }
+
+ @Test
+ public void testReplaceCommitSuccess() throws IOException {
+ TestRewriteDataFileAction testRewriteDataFileAction = new TestRewriteDataFileAction(table);
+ DataFile dataFile = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withRecordCount(10)
+ .withFileSizeInBytes(10)
+ .build();
+
+ DataFile rewrite = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withRecordCount(10)
+ .withFileSizeInBytes(10)
+ .build();
+
+ table.newFastAppend()
+ .appendFile(dataFile)
+ .commit();
+
+ table.newFastAppend()
+ .appendFile(rewrite)
+ .commit();
+
+ testRewriteDataFileAction.replaceDataFiles(Sets.newSet(dataFile), Sets.newSet(rewrite));
+
+ Assert.assertTrue(new File(dataFile.path().toString()).exists());
+ Assert.assertTrue(new File(rewrite.path().toString()).exists());
+ }
+
+ @Test
+ public void testReplaceCommitStateUnknown() throws IOException {
+ TestRewriteDataFileAction testRewriteDataFileAction = new TestRewriteDataFileAction(table);
+ DataFile dataFile = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withFileSizeInBytes(10)
+ .withRecordCount(10)
+ .build();
+
+ DataFile rewrite = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withFileSizeInBytes(10)
+ .withRecordCount(10)
+ .build();
+
+ table.newFastAppend()
+ .appendFile(dataFile)
+ .commit();
+ table.newFastAppend()
+ .appendFile(rewrite)
+ .commit();
+
+ BaseRewriteDataFilesAction spyRewriteDataFileAction = spy(testRewriteDataFileAction);
+ doThrow(new CommitStateUnknownException(new Exception("Commit-state is unknown.")))
+ .when(spyRewriteDataFileAction)
+ .commit(any());
Review comment:
I don't think that this actually tests the case that you're trying to. This throws an exception instead of calling `replaceDataFiles`. In order to test the catch clause you added, you'd need the table's rewrite action to throw the `CommitStateUnknownException`. You could do that with table spy that returns a rewrite spy that throws the exception from `commit`.
--
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
[GitHub] [iceberg] coolderli commented on a change in pull request #2621: Core: Skip delete files when commit-status is unknown.
Posted by GitBox <gi...@apache.org>.
coolderli commented on a change in pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621#discussion_r637335791
##########
File path: core/src/main/java/org/apache/iceberg/actions/BaseRewriteDataFilesAction.java
##########
@@ -267,7 +268,10 @@ private void replaceDataFiles(Iterable<DataFile> deletedDataFiles, Iterable<Data
RewriteFiles rewriteFiles = table.newRewrite();
rewriteFiles.rewriteFiles(Sets.newHashSet(deletedDataFiles), Sets.newHashSet(addedDataFiles));
commit(rewriteFiles);
- } catch (Exception e) {
+ } catch (CommitStateUnknownException e) {
+ LOG.warn("Skip delete files because we can not determine whether the commit was successful or not.");
Review comment:
Agreed, I'll fix it.
--
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
[GitHub] [iceberg] coolderli commented on pull request #2621: Core: Skip delete files when commit-status is unknown.
Posted by GitBox <gi...@apache.org>.
coolderli commented on pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621#issuecomment-846382208
@RussellSpitzer @rdblue I have added a unit test, could you help me review it?
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #2621: Core: Skip delete data files when commit-state is unknown.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621#discussion_r652135901
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBaseRewriteDataFilesAction.java
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.actions;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.CombinedScanTask;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileScanTask;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.TableTestBase;
+import org.apache.iceberg.exceptions.CommitStateUnknownException;
+import org.apache.iceberg.io.FileIO;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.mockito.internal.util.collections.Sets;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
+
+@RunWith(Parameterized.class)
+public class TestBaseRewriteDataFilesAction extends TableTestBase {
+ @Parameterized.Parameters(name = "formatVersion = {0}")
+ public static Object[] parameters() {
+ return new Object[] {1, 2};
+ }
+
+ public TestBaseRewriteDataFilesAction(int formatVersion) {
+ super(formatVersion);
+ }
+
+ @Test
+ public void testReplaceCommitSuccess() throws IOException {
+ TestRewriteDataFileAction testRewriteDataFileAction = new TestRewriteDataFileAction(table);
+ DataFile dataFile = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withRecordCount(10)
+ .withFileSizeInBytes(10)
+ .build();
+
+ DataFile rewrite = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withRecordCount(10)
+ .withFileSizeInBytes(10)
+ .build();
+
+ table.newFastAppend()
+ .appendFile(dataFile)
+ .commit();
+
+ table.newFastAppend()
+ .appendFile(rewrite)
+ .commit();
+
+ testRewriteDataFileAction.replaceDataFiles(Sets.newSet(dataFile), Sets.newSet(rewrite));
+
+ Assert.assertTrue(new File(dataFile.path().toString()).exists());
+ Assert.assertTrue(new File(rewrite.path().toString()).exists());
+ }
+
+ @Test
+ public void testReplaceCommitStateUnknown() throws IOException {
+ TestRewriteDataFileAction testRewriteDataFileAction = new TestRewriteDataFileAction(table);
+ DataFile dataFile = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withFileSizeInBytes(10)
+ .withRecordCount(10)
+ .build();
+
+ DataFile rewrite = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withFileSizeInBytes(10)
+ .withRecordCount(10)
+ .build();
+
+ table.newFastAppend()
+ .appendFile(dataFile)
+ .commit();
+ table.newFastAppend()
+ .appendFile(rewrite)
+ .commit();
+
+ BaseRewriteDataFilesAction spyRewriteDataFileAction = spy(testRewriteDataFileAction);
+ doThrow(new CommitStateUnknownException(new Exception("Commit-state is unknown.")))
+ .when(spyRewriteDataFileAction)
+ .commit(any());
Review comment:
This PR modifies `BaseRewriteDataFilesAction.replaceDataFiles`. You want to test that the update does what you expect it to. But that is not what this tests. This hijacks the call to `replaceDataFiles` not the call to `RewriteFiles.commit`, so the `RewriteFiles.commit` call never happens and the try/catch logic you updated never runs.
--
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
[GitHub] [iceberg] coolderli commented on pull request #2621: Core: Skip delete files when commit-status is unknown.
Posted by GitBox <gi...@apache.org>.
coolderli commented on pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621#issuecomment-846328710
@RussellSpitzer Ok, I'll add a unit test later.
--
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
[GitHub] [iceberg] coolderli commented on a change in pull request #2621: Core: Skip delete data files when commit-state is unknown.
Posted by GitBox <gi...@apache.org>.
coolderli commented on a change in pull request #2621:
URL: https://github.com/apache/iceberg/pull/2621#discussion_r640265298
##########
File path: core/src/test/java/org/apache/iceberg/actions/TestBaseRewriteDataFilesAction.java
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.actions;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.CombinedScanTask;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileScanTask;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.TableTestBase;
+import org.apache.iceberg.exceptions.CommitStateUnknownException;
+import org.apache.iceberg.io.FileIO;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.mockito.internal.util.collections.Sets;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
+
+@RunWith(Parameterized.class)
+public class TestBaseRewriteDataFilesAction extends TableTestBase {
+ @Parameterized.Parameters(name = "formatVersion = {0}")
+ public static Object[] parameters() {
+ return new Object[] {1, 2};
+ }
+
+ public TestBaseRewriteDataFilesAction(int formatVersion) {
+ super(formatVersion);
+ }
+
+ @Test
+ public void testReplaceCommitSuccess() throws IOException {
+ TestRewriteDataFileAction testRewriteDataFileAction = new TestRewriteDataFileAction(table);
+ DataFile dataFile = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withRecordCount(10)
+ .withFileSizeInBytes(10)
+ .build();
+
+ DataFile rewrite = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withRecordCount(10)
+ .withFileSizeInBytes(10)
+ .build();
+
+ table.newFastAppend()
+ .appendFile(dataFile)
+ .commit();
+
+ table.newFastAppend()
+ .appendFile(rewrite)
+ .commit();
+
+ testRewriteDataFileAction.replaceDataFiles(Sets.newSet(dataFile), Sets.newSet(rewrite));
+
+ Assert.assertTrue(new File(dataFile.path().toString()).exists());
+ Assert.assertTrue(new File(rewrite.path().toString()).exists());
+ }
+
+ @Test
+ public void testReplaceCommitStateUnknown() throws IOException {
+ TestRewriteDataFileAction testRewriteDataFileAction = new TestRewriteDataFileAction(table);
+ DataFile dataFile = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withFileSizeInBytes(10)
+ .withRecordCount(10)
+ .build();
+
+ DataFile rewrite = DataFiles.builder(table.spec())
+ .withFormat(FileFormat.PARQUET)
+ .withPath(Files.localOutput(temp.newFile()).location())
+ .withFileSizeInBytes(10)
+ .withRecordCount(10)
+ .build();
+
+ table.newFastAppend()
+ .appendFile(dataFile)
+ .commit();
+ table.newFastAppend()
+ .appendFile(rewrite)
+ .commit();
+
+ BaseRewriteDataFilesAction spyRewriteDataFileAction = spy(testRewriteDataFileAction);
+ doThrow(new CommitStateUnknownException(new Exception("Commit-state is unknown.")))
+ .when(spyRewriteDataFileAction)
+ .commit(any());
Review comment:
@rdblue I'm sorry I didn't catch you. I'm trying to spy spyRewriteDataFileAction, when the `commit()` function was called in `replaceDataFiles` ,it would throw the `CommitStateUnknownException`. Then I checked the exception throw and the existence of the file. I think this can also test the case. If I have something wrong, please let me know. Thanks.
--
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