From 98d25790dc97e48f17d19b2b52d0c55131338d6d Mon Sep 17 00:00:00 2001 From: Sagar Sumit Date: Fri, 5 Aug 2022 15:35:25 +0530 Subject: [PATCH] [HUDI-4550] Fallback to listing based rollback for completed instant --- .../CopyOnWriteRestoreActionExecutor.java | 1 - .../MergeOnReadRestoreActionExecutor.java | 1 - .../rollback/BaseRollbackActionExecutor.java | 10 +--- .../BaseRollbackPlanActionExecutor.java | 2 +- .../CopyOnWriteRollbackActionExecutor.java | 3 +- .../MergeOnReadRollbackActionExecutor.java | 3 +- .../hudi/client/TestClientRollback.java | 55 +++++++++++++++++++ .../TestHoodieClientOnCopyOnWriteStorage.java | 24 ++------ ...TestMergeOnReadRollbackActionExecutor.java | 19 +------ 9 files changed, 67 insertions(+), 51 deletions(-) diff --git a/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/restore/CopyOnWriteRestoreActionExecutor.java b/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/restore/CopyOnWriteRestoreActionExecutor.java index facab71c6237..f6e104e3dcdc 100644 --- a/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/restore/CopyOnWriteRestoreActionExecutor.java +++ b/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/restore/CopyOnWriteRestoreActionExecutor.java @@ -58,7 +58,6 @@ protected HoodieRollbackMetadata rollbackInstant(HoodieInstant instantToRollback instantToRollback, true, true, - false, false); return rollbackActionExecutor.execute(); } diff --git a/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/restore/MergeOnReadRestoreActionExecutor.java b/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/restore/MergeOnReadRestoreActionExecutor.java index 661cee4a2e60..01c3d44fabc9 100644 --- a/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/restore/MergeOnReadRestoreActionExecutor.java +++ b/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/restore/MergeOnReadRestoreActionExecutor.java @@ -62,7 +62,6 @@ protected HoodieRollbackMetadata rollbackInstant(HoodieInstant instantToRollback instantToRollback, true, true, - false, false); // TODO : Get file status and create a rollback stat and file diff --git a/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackActionExecutor.java b/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackActionExecutor.java index 8e34f0fe59da..96aa45ca9b95 100644 --- a/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackActionExecutor.java +++ b/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackActionExecutor.java @@ -57,7 +57,6 @@ public abstract class BaseRollbackActionExecutor partitionAndFileId1 = new HashMap() { + { + put(p1, "id11"); + put(p2, "id12"); + put(p3, "id13"); + } + }; + Map partitionAndFileId2 = new HashMap() { + { + put(p1, "id21"); + put(p2, "id22"); + put(p3, "id23"); + } + }; + Map partitionAndFileId3 = new HashMap() { + { + put(p1, "id31"); + put(p2, "id32"); + put(p3, "id33"); + } + }; + + HoodieWriteConfig config = HoodieWriteConfig.newBuilder().withPath(basePath) + .withRollbackUsingMarkers(true) // rollback using markers to test fallback to listing based rollback for completed instant + .withCleanConfig(HoodieCleanConfig.newBuilder() + .withFailedWritesCleaningPolicy(HoodieFailedWritesCleaningPolicy.LAZY).build()) + .withIndexConfig(HoodieIndexConfig.newBuilder().withIndexType(HoodieIndex.IndexType.INMEMORY).build()).build(); + + // create test table with all commits completed + HoodieTestTable testTable = HoodieMetadataTestTable.of(metaClient, SparkHoodieBackedTableMetadataWriter.create(metaClient.getHadoopConf(), config, context)); + testTable.withPartitionMetaFiles(p1, p2, p3) + .addCommit(commitTime1) + .withBaseFilesInPartitions(partitionAndFileId1) + .addCommit(commitTime2) + .withBaseFilesInPartitions(partitionAndFileId2) + .addCommit(commitTime3) + .withBaseFilesInPartitions(partitionAndFileId3); + + try (SparkRDDWriteClient client = getHoodieWriteClient(config)) { + client.rollback(commitTime3); + assertFalse(testTable.inflightCommitExists(commitTime3)); + assertFalse(testTable.baseFilesExist(partitionAndFileId3, commitTime3)); + assertTrue(testTable.baseFilesExist(partitionAndFileId2, commitTime2)); + } + } } diff --git a/hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieClientOnCopyOnWriteStorage.java b/hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieClientOnCopyOnWriteStorage.java index 28108b793aca..3f9bda49e8ff 100644 --- a/hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieClientOnCopyOnWriteStorage.java +++ b/hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieClientOnCopyOnWriteStorage.java @@ -72,19 +72,18 @@ import org.apache.hudi.common.util.StringUtils; import org.apache.hudi.common.util.collection.Pair; import org.apache.hudi.config.HoodieArchivalConfig; +import org.apache.hudi.config.HoodieCleanConfig; +import org.apache.hudi.config.HoodieClusteringConfig; import org.apache.hudi.config.HoodieCompactionConfig; import org.apache.hudi.config.HoodieIndexConfig; -import org.apache.hudi.config.HoodieClusteringConfig; +import org.apache.hudi.config.HoodiePreCommitValidatorConfig; import org.apache.hudi.config.HoodieStorageConfig; import org.apache.hudi.config.HoodieWriteConfig; -import org.apache.hudi.config.HoodiePreCommitValidatorConfig; -import org.apache.hudi.config.HoodieCleanConfig; import org.apache.hudi.data.HoodieJavaRDD; import org.apache.hudi.exception.HoodieCommitException; import org.apache.hudi.exception.HoodieCorruptedDataException; import org.apache.hudi.exception.HoodieIOException; import org.apache.hudi.exception.HoodieInsertException; -import org.apache.hudi.exception.HoodieRollbackException; import org.apache.hudi.exception.HoodieUpsertException; import org.apache.hudi.exception.HoodieValidationException; import org.apache.hudi.execution.bulkinsert.RDDCustomColumnsSortPartitioner; @@ -2297,20 +2296,9 @@ private void testRollbackAfterConsistencyCheckFailureUsingFileList(boolean rollb "With optimistic CG, first commit should succeed. commit file should be present"); // Marker directory must be removed after rollback assertFalse(metaClient.getFs().exists(new Path(metaClient.getMarkerFolderPath(instantTime)))); - if (rollbackUsingMarkers) { - // rollback of a completed commit should fail if marked based rollback is used. - try { - client.rollback(instantTime); - fail("Rollback of completed commit should throw exception"); - } catch (HoodieRollbackException e) { - // ignore - } - } else { - // rollback of a completed commit should succeed if using list based rollback - client.rollback(instantTime); - assertFalse(testTable.commitExists(instantTime), - "After explicit rollback, commit file should not be present"); - } + client.rollback(instantTime); + assertFalse(testTable.commitExists(instantTime), + "After explicit rollback, commit file should not be present"); } } diff --git a/hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/table/action/rollback/TestMergeOnReadRollbackActionExecutor.java b/hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/table/action/rollback/TestMergeOnReadRollbackActionExecutor.java index d8ce6612a443..1c4de34e5ee3 100644 --- a/hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/table/action/rollback/TestMergeOnReadRollbackActionExecutor.java +++ b/hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/table/action/rollback/TestMergeOnReadRollbackActionExecutor.java @@ -45,9 +45,9 @@ import org.apache.hudi.table.HoodieTable; import org.apache.hudi.table.marker.WriteMarkersFactory; import org.apache.hudi.testutils.MetadataMergeWriteStatus; + import org.apache.spark.api.java.JavaRDD; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -89,7 +89,7 @@ public void tearDown() throws Exception { } @ParameterizedTest - @ValueSource(booleans = {true, false}) + @ValueSource(booleans = {true}) public void testMergeOnReadRollbackActionExecutor(boolean isUsingMarkers) throws IOException { //1. prepare data and assert data result List firstPartitionCommit2FileSlices = new ArrayList<>(); @@ -281,21 +281,6 @@ public void testRollbackForCanIndexLogFile() throws IOException { assertEquals(1, partitionMetadata.getSuccessDeleteFiles().size()); } - @Test - public void testFailForCompletedInstants() { - Assertions.assertThrows(IllegalArgumentException.class, () -> { - HoodieInstant rollBackInstant = new HoodieInstant(false, HoodieTimeline.DELTA_COMMIT_ACTION, "002"); - new MergeOnReadRollbackActionExecutor(context, getConfigBuilder().build(), - getHoodieTable(metaClient, getConfigBuilder().build()), - "003", - rollBackInstant, - true, - true, - true, - false).execute(); - }); - } - /** * Test Cases for rolling back when there is no base file. */