codope commented on a change in pull request #4605:
URL: https://github.com/apache/hudi/pull/4605#discussion_r788427250
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java
##########
@@ -348,6 +349,13 @@ public HoodieTimeline getRollbackTimeline() {
return getActiveTimeline().getRollbackTimeline();
}
+ /**
+ * Get restore timeline.
+ */
+ public HoodieTimeline getRestoreTimeline() {
Review comment:
Is there a need for this API? Can we not do
`table.getActiveTimeline().getRestoreTimeline()` in the action executor?
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java
##########
@@ -391,6 +396,20 @@ public HoodieInstant
transitionRollbackRequestedToInflight(HoodieInstant request
return inflight;
}
+ /**
+ * Transition Restore State from requested to inflight.
+ *
+ * @param requestedInstant requested instant
+ * @return commit instant
+ */
+ public HoodieInstant transitionRestoreRequestedToInflight(HoodieInstant
requestedInstant) {
+
ValidationUtils.checkArgument(requestedInstant.getAction().equals(HoodieTimeline.RESTORE_ACTION));
Review comment:
Let's add meanigful message for validation failures.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java
##########
@@ -674,16 +675,21 @@ public HoodieRestoreMetadata restoreToInstant(final
String instantTime) throws H
Timer.Context timerContext = metrics.getRollbackCtx();
try {
HoodieTable<T, I, K, O> table = createTable(config, hadoopConf,
config.isMetadataTableEnabled());
- HoodieRestoreMetadata restoreMetadata = table.restore(context,
restoreInstantTime, instantTime);
- if (timerContext != null) {
- final long durationInMs = metrics.getDurationInMs(timerContext.stop());
- final long totalFilesDeleted =
restoreMetadata.getHoodieRestoreMetadata().values().stream()
- .flatMap(Collection::stream)
- .mapToLong(HoodieRollbackMetadata::getTotalFilesDeleted)
- .sum();
- metrics.updateRollbackMetrics(durationInMs, totalFilesDeleted);
+ Option<HoodieRestorePlan> restorePlanOption =
table.scheduleRestore(context, restoreInstantTime, instantTime);
Review comment:
If i understand correctly, then the assumption is still that no other
writer is inflight when restore is going to be scheduled or executed, and that
the restore is atomic i.e. all or none of the instants in the restore plan are
rolled back?
Just curious, what happens if user schedules another restore while one was
in progress? Will it even generate a new plan given that no writes were
committed in between? If the plan will be generated then can it go inflight
when another is in progress?
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieTimeline.java
##########
@@ -353,6 +354,10 @@ static HoodieInstant
getRollbackRequestedInstant(HoodieInstant instant) {
return instant.isRequested() ? instant :
HoodieTimeline.getRequestedInstant(instant);
}
+ static HoodieInstant getRestoreRequestedInstant(HoodieInstant instant) {
Review comment:
Where is this being used?
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/restore/BaseRestoreActionExecutor.java
##########
@@ -65,29 +71,53 @@ public HoodieRestoreMetadata execute() {
HoodieTimer restoreTimer = new HoodieTimer();
restoreTimer.startTimer();
- // Get all the commits on the timeline after the provided commit time
- List<HoodieInstant> instantsToRollback =
table.getActiveTimeline().getWriteTimeline()
- .getReverseOrderedInstants()
- .filter(instant ->
HoodieActiveTimeline.GREATER_THAN.test(instant.getTimestamp(),
restoreInstantTime))
- .collect(Collectors.toList());
+ Option<HoodieInstant> restoreInstant = table.getRestoreTimeline()
+ .filterInflightsAndRequested()
+ .filter(instant -> instant.getTimestamp().equals(instantTime))
+ .firstInstant();
+ if (!restoreInstant.isPresent()) {
+ throw new HoodieRollbackException("No pending restore instants found to
execute restore");
Review comment:
Would `instantTime` and `restoreInstantTime` in the execption message be
helpful?
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/restore/RestoreUtils.java
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.restore;
+
+import org.apache.hudi.avro.model.HoodieRestorePlan;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieInstant;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.table.timeline.TimelineMetadataUtils;
+
+import java.io.IOException;
+
+public class RestoreUtils {
+
+ /**
+ * Get Latest version of Restore plan corresponding to a restore instant.
+ *
+ * @param metaClient Hoodie Table Meta Client
+ * @param restoreInstant Instant referring to restore action
+ * @return Rollback plan corresponding to rollback instant
+ * @throws IOException
+ */
+ public static HoodieRestorePlan getRestorePlan(HoodieTableMetaClient
metaClient, HoodieInstant restoreInstant)
+ throws IOException {
+ // TODO: add upgrade step if required.
Review comment:
This upgrade step would only be required if users upgrade Hudi while
there is a pending restore in timeline. Is my understanding correct?
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/restore/BaseRestoreActionExecutor.java
##########
@@ -99,12 +129,23 @@ private HoodieRestoreMetadata finishRestore(Map<String,
List<HoodieRollbackMetad
writeToMetadata(restoreMetadata);
table.getActiveTimeline().saveAsComplete(new HoodieInstant(true,
HoodieTimeline.RESTORE_ACTION, instantTime),
TimelineMetadataUtils.serializeRestoreMetadata(restoreMetadata));
+ // get all rollbacks instants after restore instant time and delete them.
Review comment:
change comment to be more clear: "get all **pending** rollbacks
instants..."
--
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]
