[HUDI-839] Introducing support for rollbacks using marker files #1756

lw309637554 · 2020-06-22T14:48:41Z

Tips

Thank you very much for contributing to Apache Hudi.
Please review https://hudi.apache.org/contributing.html before opening a pull request.

What is the purpose of the pull request

add more tests for MarkerFiles,RollbackUtils, RollbackActionExecutor for markers and filelisting.
also fix the bugs for RollbackActionExecutor with markers mode or filelisting mode:

in HoodieWriteClient.java should not deleteMarkerDir, the rollback with markerfiles mode will failed
in ListingBasedRollbackHelper.java "(path.toString().endsWith(HoodieFileFormat.HOODIE_LOG.getFileExtension()))" can not check file is logfile
in "MarkerBasedRollbackStrategy.java" baseCommitTime should use FSUtils.getCommitTime(baseFilePathForAppend.getName());

Brief change log

(for example:)

Modify AnnotationLocation checkstyle rule in checkstyle.xml

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.

- Initial commit

…tionExecutor for markers and filelisting

vinothchandar · 2020-06-24T00:08:48Z

@lw309637554 is this ready for review? seems like we have unit test failures?

...c/test/java/org/apache/hudi/table/action/rollback/TestCopyOnWriteRollbackActionExecutor.java

vinothchandar

Took a quick pass at the three test classes you have added.. LGTM .
Will do a detailed pass once you confirm PR is indeed ready..

lw309637554 · 2020-06-24T02:18:31Z

Took a quick pass at the three test classes you have added.. LGTM .
Will do a detailed pass once you confirm PR is indeed ready..

Thanks,i will fix the failed unit test

…ng markers, fix bugs

lw309637554 · 2020-06-26T09:58:40Z

Took a quick pass at the three test classes you have added.. LGTM .
Will do a detailed pass once you confirm PR is indeed ready..

@vinothchandar hello，i have add more test for end to end rollback. And the all hudi unit test passed.
There are a few questions to discuss with you:

for rollback successful commit, in HoodieWriteClient.java i remove the deleteMarkerDir() in postcommit when is in usingmarkers mode. But it will double the file numbers in dfs.
if the markers file retain, if we should clean it when the datafile is cleaned, also if we should archive the markers file when archiveCommitsWith

if these two question clear and solved, i think the patch will be ok .

vinothchandar · 2020-06-28T10:07:43Z

for rollback successful commit, in HoodieWriteClient.java i remove the deleteMarkerDir() in postcommit when is in usingmarkers mode. But it will double the file numbers in dfs.

I think delaying marker deletion till cleaning is probably ok. but the reconcilation with data files i.e the deletion of extraeneous data files written due to spark stage retries must be handled pre-commit..

if the markers file retain, if we should clean it when the datafile is cleaned, also if we should archive the markers file when archiveCommitsWith

there is no need to archive teh marker files in my opinion.. the contract in Hudi is that once an instant leaves the active timeline, its effects are permanent on the table ... so if a rollback needs to happen based on marker files, then it needs to be within the retained commits for active timeline.. I think this is a practical approach..

think of active timeline as the transaction log with pending actions/inflight/completed actions..

lw309637554 · 2020-07-01T17:18:46Z

for rollback successful commit, in HoodieWriteClient.java i remove the deleteMarkerDir() in postcommit when is in usingmarkers mode. But it will double the file numbers in dfs.

I think delaying marker deletion till cleaning is probably ok. but the reconcilation with data files i.e the deletion of extraeneous data files written due to spark stage retries must be handled pre-commit..

if the markers file retain, if we should clean it when the datafile is cleaned, also if we should archive the markers file when archiveCommitsWith

there is no need to archive teh marker files in my opinion.. the contract in Hudi is that once an instant leaves the active timeline, its effects are permanent on the table ... so if a rollback needs to happen based on marker files, then it needs to be within the retained commits for active timeline.. I think this is a practical approach..

think of active timeline as the transaction log with pending actions/inflight/completed actions..

thanks, agree with you . I will update the PR.

vinothchandar · 2020-07-01T17:55:13Z

I will do a deep review of this over the weekend.. and see if I fix few things and merge.. thanks @lw309637554 !

lw309637554 · 2020-07-03T00:22:17Z

I will do a deep review of this over the weekend.. and see if I fix few things and merge.. thanks @lw309637554 !

Thanks, i add the markerfiles clean in pre-commit for spark stage retries.

vinothchandar

Thanks for adding the tests.. left some comments..

I am trying to cross check the deletion of the marker dir once more (the issue you mentioned before in prev iteration).. I will push an update to the branch.. and it should be good to go.

vinothchandar · 2020-07-04T00:03:12Z

hudi-client/src/main/java/org/apache/hudi/client/HoodieWriteClient.java

-      Option<Map<String, String>> extraMetadata) {
+  protected void postCommit(HoodieTable<?> table, HoodieCommitMetadata metadata, String instantTime, Option<Map<String, String>> extraMetadata) {
    try {
+      if (!config.getRollBackUsingMarkers()) {


I am wondering if we should do this always.. having this logic be rollback dependent can become hard to reason with in the long run

i also think it not so good. i think can do not delete markerfiles here.Can delete the unuseful markerfile in pre-commit. when clean delete the old markerfiles

hudi-client/src/main/java/org/apache/hudi/io/HoodieAppendHandle.java

hudi-client/src/main/java/org/apache/hudi/io/HoodieMergeHandle.java

hudi-client/src/main/java/org/apache/hudi/io/HoodieWriteHandle.java

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

hudi-client/src/test/java/org/apache/hudi/index/TestHoodieIndex.java

hudi-client/src/test/java/org/apache/hudi/table/TestCleaner.java

hudi-client/src/test/java/org/apache/hudi/table/TestHoodieMergeOnReadTable.java

...c/test/java/org/apache/hudi/table/action/rollback/TestCopyOnWriteRollbackActionExecutor.java

...c/test/java/org/apache/hudi/table/action/rollback/TestMergeOnReadRollbackActionExecutor.java

lw309637554 · 2020-07-06T01:53:59Z

Thanks for adding the tests.. left some comments..

I am trying to cross check the deletion of the marker dir once more (the issue you mentioned before in prev iteration).. I will push an update to the branch.. and it should be good to go.

thanks , i will fix the issue as the comment. More comprehensive solution about delete markerfile will depend on you

lw309637554

@vinothchandar I have fix all the issue as you comment. More comprehensive solution about delete markerfile will depend on you

...c/test/java/org/apache/hudi/table/action/rollback/TestCopyOnWriteRollbackActionExecutor.java

hudi-client/src/main/java/org/apache/hudi/io/HoodieWriteHandle.java

lw309637554 · 2020-07-07T06:26:06Z

hudi-client/src/test/java/org/apache/hudi/client/TestTableSchemaEvolution.java

+  }
+
+  @Test
+  public void testCopyOnWriteTableUsingMarkersRollBack() throws Exception {


yes ,i will remove it

lw309637554 · 2020-07-07T15:26:39Z

@vinothchandar I have fix all the issue as you comment. More comprehensive solution about delete markerfile will depend on you

vinothchandar · 2020-07-07T22:39:49Z

Thanks! on it!

vinothchandar · 2020-07-15T19:09:00Z

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

@lw309637554 any reason for changing this to PARQUET? (or was that me?)

Get it. it is because master merge do not handle well. Let me fix it

vinothchandar · 2020-07-16T18:14:51Z

thanks @lw309637554 actually was working on fixing it myself :). I am working on the marker dir deletion now. Will push to this branch later today.

lw309637554 · 2020-07-17T00:11:56Z

thanks @lw309637554 actually was working on fixing it myself :). I am working on the marker dir deletion now. Will push to this branch later today.

okay

- Added marker dir deletion after successful commit/rollback, individual files are not deleted during finalize - Fail safe for deleting marker directories, now during timeline archival process - Added check to ensure completed instants are not rolled back using marker based strategy. This will be incorrect - Reworked tests to rollback inflight instants, instead of completed instants whenever necessary - Added an unit test for MarkerBasedRollbackStrategy

vinothchandar

@lw309637554 I pushed the changes to the branch.. Once CI passes, this should be good to merge IMO. Please take a pass at teh changes I made in the last two commits..

@bvaradar @n3nash this is a tricky change. Since you both know rollback and marker files the best. can you make a quick pass of the places I have highlighted in the review.

vinothchandar · 2020-07-19T22:54:31Z

hudi-client/src/main/java/org/apache/hudi/client/HoodieWriteClient.java

+  protected void postCommit(HoodieTable<?> table, HoodieCommitMetadata metadata, String instantTime, Option<Map<String, String>> extraMetadata) {
    try {
+
+      // Delete the marker directory for the instant.


this PR will change behavior for marker dir deletion, with or without marker based rollback turned on.

vinothchandar · 2020-07-19T22:55:31Z

hudi-client/src/main/java/org/apache/hudi/io/HoodieWriteHandle.java

-  /**
-   * THe marker path will be <base-path>/.hoodie/.temp/<instant_ts>/2019/04/25/filename.
-   */
-  private Path makeNewMarkerPath(String partitionPath) {


all of this stuff is now encapsulatd into a MarkerFiles class

vinothchandar · 2020-07-19T22:56:43Z

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

-          baseFileExtension);
-      List<String> validDataPaths = stats.stream().map(w -> String.format("%s/%s", basePath, w.getPath()))
-          .filter(p -> p.endsWith(baseFileExtension)).collect(Collectors.toList());
+      // we are not including log appends here, since they are already fail-safe.


quick skim of changes here would be nice.

This looks good to me.

hudi-client/src/main/java/org/apache/hudi/table/HoodieTimelineArchiveLog.java

vinothchandar · 2020-07-19T22:58:30Z

hudi-client/src/main/java/org/apache/hudi/table/MarkerFiles.java

+  /**
+   * Delete Marker directory corresponding to an instant.
+   */
+  public boolean deleteMarkerDir() {


@umehrot2 we can now add the parallelization changes here. for deletion of marker files.

vinothchandar · 2020-07-19T23:00:10Z

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

+    return deleteBaseFile(createdBaseFilePath);
+  }
+
+  private HoodieRollbackStat deleteBaseFile(String baseFilePath) throws IOException {


this is resilient already to attempting to delete an non-existent file.. marker file may not imply the data file is thre.

vinothchandar · 2020-07-19T23:00:46Z

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

+            .withPartitionPath(partitionPath)
+            .build();
+      }
+      writer = HoodieLogFormat.newWriterBuilder()


checked that the log scanner can deal with spurious rollback blocks.. i.e rollbacks logged without any data blocks for that instant

vinothchandar · 2020-07-20T03:23:31Z

test is failing on CI (linux?) while it passes locally.. Looking.

bvaradar

Made a pass. @vinothchandar : Code changes look safe and good to me.

bvaradar · 2020-07-20T03:02:37Z

hudi-client/src/main/java/org/apache/hudi/client/HoodieWriteClient.java

+  protected void postCommit(HoodieTable<?> table, HoodieCommitMetadata metadata, String instantTime, Option<Map<String, String>> extraMetadata) {
    try {
+
+      // Delete the marker directory for the instant.


hudi-client/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java

bvaradar · 2020-07-20T03:33:18Z

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

-          baseFileExtension);
-      List<String> validDataPaths = stats.stream().map(w -> String.format("%s/%s", basePath, w.getPath()))
-          .filter(p -> p.endsWith(baseFileExtension)).collect(Collectors.toList());
+      // we are not including log appends here, since they are already fail-safe.


This looks good to me.

hudi-client/src/main/java/org/apache/hudi/table/HoodieTimelineArchiveLog.java

hudi-client/src/main/java/org/apache/hudi/table/MarkerFiles.java

vinothchandar · 2020-07-21T04:56:59Z

@lw309637554 Looks good. Planning to merge after CI passes this time.. Thanks a lot of for your contributions. This is one very important PR !

lw309637554 · 2020-07-21T07:38:32Z

@lw309637554 Looks good. Planning to merge after CI passes this time.. Thanks a lot of for your contributions. This is one very important PR !

Great, a very good cooperation experience . I can start other work

vinothchandar and others added 3 commits June 1, 2020 19:47

[HUDI-839] Introducing rollback strategy using marker files

922b1ef

- Initial commit

Adding unit test for MarkerFiles

470842d

[HUDI-839] Adding unit test for MarkerFiles,RollbackUtils, RollbackAc…

751f2ba

…tionExecutor for markers and filelisting

leesf assigned vinothchandar Jun 22, 2020

[HUDI-839] Merge branch 'master'

e55ce7c

lw309637554 force-pushed the HUDI-839-lw branch from d7831d8 to e55ce7c Compare June 22, 2020 16:15

vinothchandar changed the title ~~Adding unit test for MarkerFiles,RollbackUtils, RollbackActionExecutor for markers and filelisting~~ [HUDI-839] Adding unit test for MarkerFiles,RollbackUtils, RollbackActionExecutor for markers and filelisting Jun 24, 2020

vinothchandar reviewed Jun 24, 2020

View reviewed changes

...c/test/java/org/apache/hudi/table/action/rollback/TestCopyOnWriteRollbackActionExecutor.java Outdated Show resolved Hide resolved

vinothchandar reviewed Jun 24, 2020

View reviewed changes

[HUDI-839] merge latest master patch , add more test for rollback usi…

3724403

…ng markers, fix bugs

lw309637554 force-pushed the HUDI-839-lw branch from 6984b05 to 3724403 Compare June 25, 2020 17:25

[HUDI-839] merge latest master

97380ea

vinothchandar mentioned this pull request Jun 28, 2020

[WIP] [HUDI-839] Introducing rollback strategy using marker files #1593

Closed

5 tasks

lw309637554 added 2 commits July 2, 2020 16:06

[HUDI-839] clean marker file at pre commit for spark retries

713dd31

[HUDI-839] merge latest master

9f57d75

vinothchandar reviewed Jul 6, 2020

View reviewed changes

lw309637554 added 2 commits July 7, 2020 16:16

[HUDI-839] using fileformat with table.getBaseFileExtension

1354336

Merge branch 'master' into HUDI-839-lw

d77c0e3

lw309637554 force-pushed the HUDI-839-lw branch 2 times, most recently from 0a896c6 to 07558e7 Compare July 7, 2020 15:16

lw309637554 commented Jul 7, 2020

View reviewed changes

vinothchandar reviewed Jul 15, 2020

View reviewed changes

lw309637554 force-pushed the HUDI-839-lw branch from 07558e7 to f81f27c Compare July 16, 2020 02:38

[HUDI-839] revert the useless unit tests

d66a9ec

lw309637554 force-pushed the HUDI-839-lw branch from f81f27c to d66a9ec Compare July 16, 2020 05:54

vinothchandar added 2 commits July 19, 2020 15:41

Merge branch 'master' into pull/1756

a0c4de4

vinothchandar reviewed Jul 19, 2020

View reviewed changes

vinothchandar force-pushed the HUDI-839-lw branch from 0e5a0bf to 37c1feb Compare July 20, 2020 00:47

vinothchandar changed the title ~~[HUDI-839] Adding unit test for MarkerFiles,RollbackUtils, RollbackActionExecutor for markers and filelisting~~ [HUDI-839] Introducing support for rollbacks using marker files Jul 20, 2020

bvaradar reviewed Jul 20, 2020

View reviewed changes

vinothchandar force-pushed the HUDI-839-lw branch 4 times, most recently from c6afcb1 to 6371ace Compare July 20, 2020 22:13

Fix compilation issue & 1 unit test failure

551ce84

vinothchandar force-pushed the HUDI-839-lw branch from 6371ace to 551ce84 Compare July 20, 2020 23:34

vinothchandar added 2 commits July 20, 2020 20:20

Fix small bug in test utils for casing test file correctly

e7ab8f3

Remove unused method on HoodieWriteConfig

f36b234

vinothchandar merged commit 1ec89e9 into apache:master Jul 21, 2020

wangxianghu mentioned this pull request Sep 8, 2020

[HUDI-1089] Refactor hudi-client to support multi-engine #1827

Merged

5 tasks

[HUDI-839] Introducing support for rollbacks using marker files #1756

[HUDI-839] Introducing support for rollbacks using marker files #1756

Uh oh!

Conversation

lw309637554 commented Jun 22, 2020

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

vinothchandar commented Jun 24, 2020

Uh oh!

Uh oh!

vinothchandar left a comment

Choose a reason for hiding this comment

Uh oh!

lw309637554 commented Jun 24, 2020

Uh oh!

lw309637554 commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vinothchandar commented Jun 28, 2020

Uh oh!

lw309637554 commented Jul 1, 2020

Uh oh!

vinothchandar commented Jul 1, 2020

Uh oh!

lw309637554 commented Jul 3, 2020

Uh oh!

vinothchandar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lw309637554 commented Jul 6, 2020

Uh oh!

lw309637554 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lw309637554 commented Jul 7, 2020

Uh oh!

vinothchandar commented Jul 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinothchandar commented Jul 16, 2020

Uh oh!

lw309637554 commented Jul 17, 2020

Uh oh!

vinothchandar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lw309637554 commented Jun 26, 2020 •

edited

Loading

lw309637554 left a comment •

edited

Loading