nsivabalan commented on code in PR #8849:
URL: https://github.com/apache/hudi/pull/8849#discussion_r1212454576
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackActionExecutor.java:
##########
@@ -240,22 +244,21 @@ protected void finishRollback(HoodieInstant
inflightInstant, HoodieRollbackMetad
this.txnManager.beginTransaction(Option.of(inflightInstant),
Option.empty());
}
- // If publish the rollback to the timeline, we first write the rollback
metadata
- // to metadata table
+ // If publish the rollback to the timeline, we first write the rollback
metadata to metadata table
+ // Then transition the inflight rollback to completed state.
if (!skipTimelinePublish) {
writeTableMetadata(rollbackMetadata);
- }
-
- // Then we delete the inflight instant in the data table timeline if
enabled
- deleteInflightAndRequestedInstant(deleteInstants,
table.getActiveTimeline(), resolvedInstant);
-
- // If publish the rollback to the timeline, we finally transition the
inflight rollback
- // to complete in the data table timeline
- if (!skipTimelinePublish) {
table.getActiveTimeline().transitionRollbackInflightToComplete(inflightInstant,
TimelineMetadataUtils.serializeRollbackMetadata(rollbackMetadata));
LOG.info("Rollback of Commits " +
rollbackMetadata.getCommitsRollback() + " is complete");
}
+
+ // Commit to rollback instant files are deleted after the rollback
commit is transitioned from inflight to completed
+ // If job were to fail after transitioning rollback from inflight to
complete and before delete the instant files,
+ // then subsequent retries of the rollback for this instant will see if
there is a completed rollback present for this instant
+ // and then directly delete the files and abort.
Review Comment:
can you point me to this part of the code. Where we check for completed
rollback instants and clean up the files directly ?
From what I know, we only check for pending rollbacks
ref: getPendingRollbackInfos
and re-use the rollback instant if applicable.
Also, do we have a test for this ?
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/table/action/rollback/TestCopyOnWriteRollbackActionExecutor.java:
##########
@@ -328,4 +340,213 @@ private void performRollbackAndValidate(boolean
isUsingMarkers, HoodieWriteConfi
assertFalse(WriteMarkersFactory.get(cfg.getMarkersType(), table,
commitInstant.getTimestamp()).doesMarkerDirExist());
}
+
+ /**
+ * This method tests rollback of completed ingestion commits and
replacecommit inflight files
+ * when there is another replacecommit with greater timestamp already
present in the timeline.
+ */
+ @Test
+ public void testRollbackWhenReplaceCommitIsPresent() throws Exception {
Review Comment:
can you move these into separate PRs. will help landing the PRs at diff
pace. also, the PR description will be succinct. its fine for this PR. but in
future
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/table/action/cluster/ClusteringTestBase.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.hudi.table.action.cluster;
+
+import org.apache.hudi.client.SparkRDDWriteClient;
+import org.apache.hudi.client.validator.SqlQueryEqualityPreCommitValidator;
+import org.apache.hudi.common.config.HoodieStorageConfig;
+import org.apache.hudi.common.fs.ConsistencyGuardConfig;
+import org.apache.hudi.common.model.HoodieFailedWritesCleaningPolicy;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.versioning.TimelineLayoutVersion;
+import org.apache.hudi.common.table.view.FileSystemViewStorageConfig;
+import org.apache.hudi.common.table.view.FileSystemViewStorageType;
+import org.apache.hudi.common.testutils.HoodieTestDataGenerator;
+import org.apache.hudi.common.util.Option;
+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.HoodiePreCommitValidatorConfig;
+import org.apache.hudi.config.HoodieWriteConfig;
+import org.apache.hudi.index.HoodieIndex;
+import org.apache.hudi.testutils.HoodieClientTestUtils;
+import org.apache.hudi.testutils.MetadataMergeWriteStatus;
+
+import java.util.Properties;
+
+public class ClusteringTestBase {
+ protected static int timelineServicePort =
+ FileSystemViewStorageConfig.REMOTE_PORT_NUM.defaultValue();
+ private static final String COUNT_SQL_QUERY_FOR_VALIDATION = "select
count(*) from <TABLE_NAME>";
Review Comment:
yeah. lets try to extends from one of the existing test bases.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
