[HUDI-2422] Adding rollback plan and rollback requested instant #3651

nsivabalan · 2021-09-13T03:48:00Z

What is the purpose of the pull request

Adding rollback plan and rollback requested instant.
We are introducing RollbackPlanning phase which will collect all file info that needs to be deleted as part of the rollback action. This HoodieRollBackPlan will get serialized into commitInstant.rollback.requested.
- As of of this, adding a new scheduleRollback() method to the HoodieTable. Any rollback, should first schedule and then trigger actual rollback.

  public abstract Option<HoodieRollbackPlan> scheduleRollback(HoodieEngineContext context,
                                                              String instantTime,
                                                              HoodieInstant instantToRollback,
                                                              boolean skipTimelinePublish);

During rollback action phase, state is transitioned to inflight and the original rollback plan will be used. In this action phase, no listing or marker based fetch is performed. All we do in this phase is, fetch all file info to be deleted from serialized rollback plan and delete them. For log file deletions, we make appends to log files.

Brief change log

Added a new avro model for RollbackPlan (which contains a list of HoodieRollbackRequests).
Rollback strategy interface is changes as below.

  interface RollbackStrategy extends Serializable {
    /**
     * Fetch list of {@link HoodieRollbackRequest}s to be added to rollback plan.
     * @param instantToRollback instant to be rolled back.
     * @return list of {@link HoodieRollbackRequest}s to be added to rollback plan
     */
    List<HoodieRollbackRequest> getRollbackRequest(HoodieInstant instantToRollback);
  }

As part of this change, RollBackStrategy is moved from RollbackActionExecutor to RollbackPlanExecutor. Also, have added separate strategy class for Listing bases strategy.
Classes added or modified relating to Planning phase
- BaseRollbackPlanActionExecutor : Create the Rollback plan (leveraging the right strategy) and serialize into rollback.requested meta file.
  - Inherited by SparkCOW/SparkMORRollbackPlanActionExecutor, FlinkCOW/FlinkMORRollbackPlanActionExecutor, and JavaCOWRollbackPlanActionExecutor.
- AbstractMarkerBasedRollbackStrategy and ListingBasedRollbackStrategy implements BaseRollbackPlanActionExecutor.RollbackStrategy.
  - ListingBased: SparkCOW/SparkMORListingBasedRollbackStrategy, FlinkCOW/FlinkMORListingBasedRollbackStrategy and JavaCOWListingBasedRollbackStrategy.
  - BaseMarkerBasedRollbackStrategy(used by both Flink and Java) and SparkMarkerBasedRollbackStrategy
Classes modified for rollback action phase:
- BaseRollbackActionExecutor: where the actual rollback happens.
  - BaseCopyOnWriteRollbackActionExecutor(used by all 3 engines) and BaseMergeOnReadRollbackActionExecutor (used by all 3 engines)
    - Spark/Flink/JavaCOWRollbackActionExecutor and Spark/FlinkMORRollbackActionExecutor
- Have added RollbackHelper classes to re-use code as much as possible across all 3 engines.
  - BaseRollbackHelpers: Spark/Flink/JavaRollbackHelper
  - Spark/Java/FlinkListingBasedRollbackHelper classes contains methods used in listing based rollback strategy.
Have fixed upgrade classes to also use the new approach
Fixed all existing tests relating to rollback to invoke both rollbackPlan followed by rollback action.
Tracking few follow ups here https://issues.apache.org/jira/browse/HUDI-2422

Verify this pull request

(Please pick either of the following options)

This pull request is a trivial rework / code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end.
Added HoodieClientWriteTest to verify the change.
Manually verified the change by running a job locally.

Committer checklist

Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

hudi-bot · 2021-09-13T03:51:10Z

CI report:

de4e738 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run travis re-run the last Travis build
@hudi-bot run azure re-run the last Azure build

...mmon/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackPlanActionExecutor.java

vinothchandar

Left some high level comments

vinothchandar · 2021-09-13T13:48:19Z

...lient/hudi-client-common/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java

@@ -590,6 +591,11 @@ public boolean rollback(final String commitInstantTime) throws HoodieRollbackExc
          .filter(instant -> HoodieActiveTimeline.EQUALS.test(instant.getTimestamp(), commitInstantTime))
          .findFirst());
      if (commitInstantOpt.isPresent()) {
+
+        LOG.info("Scheduling Rollback at instant time :" + rollbackInstantTime);
+        Option<HoodieRollbackPlan> rollbackPlan = createTable(config, hadoopConf)


if this follows convention of how we do compaction scheduling, i think its good.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java

...mmon/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackPlanActionExecutor.java

vinothchandar · 2021-09-13T13:52:40Z

hudi-common/src/main/avro/HoodieListingBasedRollbackRequest.avsc

+{
+  "namespace": "org.apache.hudi.avro.model",
+  "type": "record",
+  "name": "HoodieListingBasedRollbackRequest",


why does the plan care about listing vs marker based for the plan serialization itself?

Could be just store generically what file paths need to be deleted and where command blocks need to be logged? then the plan becomes the source of truth

nsivabalan · 2021-09-13T04:03:10Z

...lient/hudi-flink-client/src/main/java/org/apache/hudi/table/HoodieFlinkCopyOnWriteTable.java

+  @Override
+  public Option<HoodieRollbackPlan> scheduleRollback(HoodieEngineContext context, String instantTime, HoodieInstant instantToRollback,
+                                                     boolean skipTimelinePublish) {
+    return null;


yet to fix flink and java. will be fixing it in a day or two.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java

...mmon/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackPlanActionExecutor.java

...k-client/src/main/java/org/apache/hudi/table/action/rollback/ListingBasedRollbackHelper.java

nsivabalan · 2021-09-14T05:55:13Z

hudi-common/src/main/avro/HoodieListingBasedRollbackRequest.avsc

+{
+  "namespace": "org.apache.hudi.avro.model",
+  "type": "record",
+  "name": "HoodieListingBasedRollbackRequest",


nsivabalan · 2021-09-14T17:32:48Z

...t-common/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackActionExecutor.java

@@ -53,11 +54,6 @@

  private static final Logger LOG = LogManager.getLogger(BaseRollbackActionExecutor.class);

-  interface RollbackStrategy extends Serializable {


Note to reviewer: RollbackStrategy is now moved to planActionExecutor and also interface methods have changed.

...c/main/java/org/apache/hudi/table/action/rollback/JavaCopyOnWriteRollbackActionExecutor.java

nsivabalan · 2021-09-14T17:42:12Z

...rc/main/java/org/apache/hudi/table/action/restore/SparkCopyOnWriteRestoreActionExecutor.java

@@ -48,20 +48,23 @@ public SparkCopyOnWriteRestoreActionExecutor(HoodieSparkEngineContext context,

  @Override
  protected HoodieRollbackMetadata rollbackInstant(HoodieInstant instantToRollback) {
+    if (!instantToRollback.getAction().equals(HoodieTimeline.COMMIT_ACTION)


moved code from below to here.

nsivabalan

Added some notes for reviewer

vinothchandar

High level code structure makes sense to me.

vinothchandar · 2021-09-15T03:56:33Z

...lient/hudi-client-common/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java

@@ -590,6 +590,10 @@ public boolean rollback(final String commitInstantTime) throws HoodieRollbackExc
          .filter(instant -> HoodieActiveTimeline.EQUALS.test(instant.getTimestamp(), commitInstantTime))
          .findFirst());
      if (commitInstantOpt.isPresent()) {
+        LOG.info("Scheduling Rollback at instant time :" + rollbackInstantTime);
+        createTable(config, hadoopConf)


do something with the return value?

vinothchandar · 2021-09-15T04:04:44Z

...src/main/java/org/apache/hudi/table/action/rollback/AbstractMarkerBasedRollbackStrategy.java

   * @return Map<FileStatus, File size>
   * @throws IOException
   */
  protected Map<FileStatus, Long> getWrittenLogFileSizeMap(String partitionPath, String baseCommitTime, String fileId) throws IOException {
-    return Collections.EMPTY_MAP;
+    // collect all log files that is supposed to be deleted with this rollback
+    return FSUtils.getAllLogFiles(table.getMetaClient().getFs(),


should this go through metadata table for listing? would not doing so cause any issues? i.e we may log the rollback some place thats not reachable from metadata table and thus fail to interpret the rollback command

I don't fully get this point. I have created a follow up ticket here. will follow up with you.
here is my understanding: of a scenario using cloud stores that does not support append.

If there was crash during a commit, when listing log files to be logged, the last one which got crashed may not be part of the rollback plan. but thats should be fine. anyways, its not available via listing. and so I assume even during compaction those will not be available. we will proceed on with rollback by adding another log block (file). and this will get replayed to metadata table.

If you are talking about the case, where a crash happens when rollback itself is being logged and crashed just before committing to metadata table.
we should be ok here too. we will retry the rollback which will redo the action phase. and will add new log blocks (with same old logs that were part of failed writes, just that it may not be able to successfully delete). and this will get applied to metadata table. We just have to ensure when applying changes to metadata table, we consider all files from the plan and not just the ones that got successfully deleted.

vinothchandar · 2021-09-15T04:08:29Z

...c/main/java/org/apache/hudi/table/action/rollback/BaseCopyOnWriteRollbackActionExecutor.java

+   * @param rollbackPlan instance of {@link HoodieRollbackPlan} for which rollback needs to be executed.
+   * @return list of {@link HoodieRollbackStat}s.
+   */
+  abstract List<HoodieRollbackStat> rollbackAndGetStats(HoodieInstant instantToRollback, HoodieRollbackPlan rollbackPlan);


executeRollback?

vinothchandar · 2021-09-15T04:11:10Z

...mon/src/main/java/org/apache/hudi/table/action/rollback/BaseMarkerBasedRollbackStrategy.java

-            return undoCreate(WriteMarkers.stripMarkerSuffix(markerFilePath));
+            String fileToDelete = WriteMarkers.stripMarkerSuffix(markerFilePath);
+            Path fullDeletePath = new Path(basePath, fileToDelete);
+            String partitionPath = FSUtils.getRelativePartitionPath(new Path(basePath), fullDeletePath.getParent());


We should move any helper that is not actually doing anything with an fs instance to somethihg like PathUtils

https://issues.apache.org/jira/browse/HUDI-2437

vinothchandar · 2021-09-15T04:15:19Z

...t-common/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackActionExecutor.java

+        HoodieRollbackPlan rollbackPlan = RollbackUtils.getRollbackPlan(table.getMetaClient(), rollbackInstant);
+        return runRollback(table, rollBackInstants.get(0), rollbackPlan);
+      } catch (IOException e) {
+        throw new HoodieIOException(e.getMessage(), e);


add more context to the exception?

vinothchandar · 2021-09-15T04:17:41Z

...ommon/src/main/java/org/apache/hudi/table/action/rollback/HoodieRollbackRequestInternal.java

+import java.util.List;
+import java.util.Map;
+
+public class HoodieRollbackRequestInternal {


SerializableRollbackRequest

vinothchandar · 2021-09-15T04:21:02Z

hudi-common/src/main/avro/HoodieRollbackPlan.avsc

+                                       "items": "string"
+                                    }
+                            },
+                            {"name": "logFilesToBeDeleted",


rename this? log blocks to rollback? or sth?

vinothchandar · 2021-09-15T04:24:39Z

...k-client/src/main/java/org/apache/hudi/table/action/rollback/ListingBasedRollbackHelper.java

-/**
- * Performs Rollback of Hoodie Tables.
- */
-public class ListingBasedRollbackHelper implements Serializable {


can bunch of this live in client-common?

vinothchandar · 2021-09-15T04:27:49Z

...java/org/apache/hudi/table/action/rollback/SparkCopyOnWriteListingBasedRollbackStrategy.java

+public class SparkCopyOnWriteListingBasedRollbackStrategy extends ListingBasedRollbackStrategy {
+
+  public SparkCopyOnWriteListingBasedRollbackStrategy(HoodieEngineContext context,
+                                                      HoodieWriteConfig config,
+                                                      HoodieTable table,
+                                                      String instantTime) {
+    super(table, context, config, instantTime);
+  }
+
+  @Override
+  public List<HoodieRollbackRequest> getRollbackRequest(HoodieInstant instantToRollback) {
+    List<ListingBasedRollbackRequest> rollbackRequests = RollbackUtils.generateRollbackRequestsByListingCOW(context,
+        table.getMetaClient().getBasePath(), config);
+    List<HoodieRollbackRequest> listingBasedRollbackRequests = new SparkListingBasedRollbackHelper(table.getMetaClient(), config)
+        .getRollbackRequestsForRollbackPlan(context, instantToRollback, rollbackRequests);
+    return listingBasedRollbackRequests;
+  }
+}


just have one class for COW and MOR ? and branch internally using table type?

danny0405 · 2021-09-15T11:53:50Z

...mmon/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackPlanActionExecutor.java

+        }
+      }
+      return Option.of(rollbackPlan);
+    } catch (IOException e) {


The Option is never empty, there is no need to return Option here, just return rollbackPlan which is more straight-forward.

just trying to keep in sync with other operations like clean, compaction etc.

nsivabalan commented Sep 13, 2021

View reviewed changes

...mmon/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackPlanActionExecutor.java Show resolved Hide resolved

nsivabalan force-pushed the rollbackPlan branch from aefd1e0 to f74bf78 Compare September 13, 2021 04:10

vinothchandar reviewed Sep 13, 2021

View reviewed changes

nsivabalan force-pushed the rollbackPlan branch 5 times, most recently from 67a62e1 to 2687fdc Compare September 14, 2021 19:07

nsivabalan commented Sep 14, 2021

View reviewed changes

nsivabalan force-pushed the rollbackPlan branch 2 times, most recently from 87a2824 to 076a996 Compare September 14, 2021 21:28

nsivabalan commented Sep 14, 2021

View reviewed changes

nsivabalan marked this pull request as ready for review September 14, 2021 21:56

nsivabalan force-pushed the rollbackPlan branch from 076a996 to 1c37c90 Compare September 14, 2021 22:32

nsivabalan changed the title ~~[HUDI-2422] Adding rollback plan to rollback requested instant~~ [HUDI-2422] Adding rollback plan and rollback requested instant Sep 15, 2021

vinothchandar reviewed Sep 15, 2021

View reviewed changes

nsivabalan force-pushed the rollbackPlan branch from 1c37c90 to 7f511d8 Compare September 15, 2021 05:57

danny0405 reviewed Sep 15, 2021

View reviewed changes

nsivabalan force-pushed the rollbackPlan branch from 7252e81 to 9985d10 Compare September 16, 2021 13:06

Adding rollback plan and rollback requested instant

de4e738

nsivabalan force-pushed the rollbackPlan branch from 9985d10 to de4e738 Compare September 16, 2021 13:29

nsivabalan merged commit b8dad62 into apache:master Sep 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-2422] Adding rollback plan and rollback requested instant #3651

[HUDI-2422] Adding rollback plan and rollback requested instant #3651

nsivabalan commented Sep 13, 2021 •

edited

Loading

hudi-bot commented Sep 13, 2021 •

edited

Loading

vinothchandar left a comment

vinothchandar Sep 13, 2021

vinothchandar Sep 13, 2021

vinothchandar Sep 13, 2021

nsivabalan Sep 14, 2021

nsivabalan Sep 13, 2021

nsivabalan Sep 14, 2021

nsivabalan Sep 14, 2021

nsivabalan Sep 14, 2021

nsivabalan left a comment

vinothchandar left a comment

vinothchandar Sep 15, 2021

vinothchandar Sep 15, 2021

nsivabalan Sep 15, 2021

vinothchandar Sep 15, 2021

vinothchandar Sep 15, 2021

nsivabalan Sep 15, 2021

vinothchandar Sep 15, 2021

vinothchandar Sep 15, 2021

vinothchandar Sep 15, 2021

vinothchandar Sep 15, 2021

vinothchandar Sep 15, 2021

danny0405 Sep 15, 2021

nsivabalan Sep 15, 2021

		@@ -53,11 +54,6 @@

		private static final Logger LOG = LogManager.getLogger(BaseRollbackActionExecutor.class);

		interface RollbackStrategy extends Serializable {

[HUDI-2422] Adding rollback plan and rollback requested instant #3651

[HUDI-2422] Adding rollback plan and rollback requested instant #3651

Conversation

nsivabalan commented Sep 13, 2021 • edited Loading

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

hudi-bot commented Sep 13, 2021 • edited Loading

CI report:

vinothchandar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsivabalan left a comment

Choose a reason for hiding this comment

vinothchandar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsivabalan commented Sep 13, 2021 •

edited

Loading

hudi-bot commented Sep 13, 2021 •

edited

Loading