[HUDI-3435] Do not throw exception when instant to rollback does not … #4821

danny0405 · 2022-02-15T09:12:40Z

…exist in metadata table active timeline

Tips

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

What is the purpose of the pull request

After this change, when the compaction metadata commits successfully but the data set commit state switch fails, the metadata table may bookkeep the compaction files which has been rolledback(removed), but the odds are far less than the case that metadata and data set commits never happens.

And the compaction files are actually idempotent.

Have no good solution to fix this completely, maybe we should check the archive timeline for accurate check whether the instant to rollback is archived.

Finally i add a new filtering condition for metadata table archiving to address this problem.

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.

danny0405 · 2022-02-18T03:09:39Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java

-    if (config.isMetadataTableEnabled()) {
-      try (HoodieTableMetadata tableMetadata = HoodieTableMetadata.create(table.getContext(), config.getMetadataConfig(),
-          config.getBasePath(), FileSystemViewStorageConfig.SPILLABLE_DIR.defaultValue())) {
-        Option<String> latestCompactionTime = tableMetadata.getLatestCompactionTime();


Hello @nsivabalan , i see that you put the code here, is there any special reason that the data set timeline archival should be in front of the metadata latest compaction time ?

yes.
CC @prashantwason
We have documented the reasoning here https://issues.apache.org/jira/browse/HUDI-2458

With the current metadata table design, we validate the deltacommits to read for metadata table based on which completed commits are present in the dataset. So dataset should never be archiving the instants before compaction on metadata table.

Thanks, I read the code and this patch does not break the original limitations. Because the metadata table commits before dataset, with the new patch, the instants on the dataset timeline must also be in the metadata timeline, so we can still do the check for dataset instant existence.

Hello, @nsivabalan can you help confirm this ?

There can be the case where deltacommit on the metadata table succeeds but commit on dataset fails (inflight -> complete transition fails).

When the job restarts, the last commit would be rollbacked first right ? And the view of the metadata table would be fixed then, so this should not be a problem.

In any case, the metadata table was read with latest filesystem view which includes the delta commits logs files, i don't know why we address metadata compaction here because compaction does not affect the table records.

When the job restarts, the last commit would be rollbacked first right ?
The last commit failed on the dataset but succeed on the metadata table. So yes it will be rolled back on the dataset eventually - depends on the settings (EAGER vs LAZY rollbacks).

Also we need to support the readers - they need to ignore the deltacommit. There can be a delay between the failed job and the retry and readers should read consistent data during that time.

Also we need to support the readers - they need to ignore the deltacommit

That is not the case for current metadata table reader i guess, and in personal i think this restriction is too much limited, doesn't the fail rollback already fixed the metadata table then after the rollback metadata was synced ?

There can be the case where deltacommit on the metadata table succeeds but commit on dataset fails (inflight -> complete transition fails).

When the job restarts, the last commit would be rollbacked first right ? And the view of the metadata table would be fixed then, so this should not be a problem.

Last commit may not be rolled back immediately for the scenarios of single writer with async table services and multi-writer, since LAZY should be set for hoodie.cleaner.policy.failed.writes.

Sorry i don't understand why the cleaner policy can affect this, if the rollback metadata was synced, the metadata file list can be trusted right ? And the corrupt files should then be ignored automatically.

danny0405 · 2022-02-18T04:45:06Z

@hudi-bot run azure

danny0405 · 2022-02-18T05:32:18Z

@nsivabalan The PR is ready, please review if you have time ~

danny0405 · 2022-02-18T13:45:41Z

@hudi-bot run azure

nsivabalan

CC @yihua : guess this patch is addressing the issue you hit during the testing.

nsivabalan · 2022-02-18T20:35:59Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java

-    if (config.isMetadataTableEnabled()) {
-      try (HoodieTableMetadata tableMetadata = HoodieTableMetadata.create(table.getContext(), config.getMetadataConfig(),
-          config.getBasePath(), FileSystemViewStorageConfig.SPILLABLE_DIR.defaultValue())) {
-        Option<String> latestCompactionTime = tableMetadata.getLatestCompactionTime();


yes.
CC @prashantwason
We have documented the reasoning here https://issues.apache.org/jira/browse/HUDI-2458

nsivabalan · 2022-02-18T20:38:57Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java

-    if (config.isMetadataTableEnabled()) {
-      try (HoodieTableMetadata tableMetadata = HoodieTableMetadata.create(table.getContext(), config.getMetadataConfig(),
-          config.getBasePath(), FileSystemViewStorageConfig.SPILLABLE_DIR.defaultValue())) {
-        Option<String> latestCompactionTime = tableMetadata.getLatestCompactionTime();


With the current metadata table design, we validate the deltacommits to read for metadata table based on which completed commits are present in the dataset. So dataset should never be archiving the instants before compaction on metadata table.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java

nsivabalan

left comments

danny0405 · 2022-02-23T08:39:40Z

Hello @nsivabalan , i think after this path, the compaction instant constraint can be removed, can you double check this ?

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java

yihua · 2022-03-04T00:16:06Z

...i-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java

            .archiveCommitsWith(3, 4)
            .retainCommits(1)
            .build())
+        .withMarkersType("DIRECT")


What's the reason for adding this?

Misadded can be removed.

nsivabalan · 2022-03-06T20:51:04Z

@danny0405 : sorry, I don't think we can remove the data table relying on metadata table compaction. can you help me understand. Myself and @yihua did jam on this.

here is our claim
In metadata table, we filter out any additional commits which is complete is MDT, but not yet complete in Data table. so, if we loosen the dependency, there could be a partially failed commit in data table which gets archived. In which case when we inpsect the commit in MDT, we might assume that its already committed in data table as well which could lead to wrong results.

If I am missing something, let me know.

danny0405 · 2022-03-07T08:32:46Z

there could be a partially failed commit in data table which gets archived

For 'partially failed commit' do you mean the commit in the data set table ? Then it should not goes to the step of the archive i think, we do archive post of a successful data set commit.

And when we have a commit t1, and t1 commit to metadata table successfully but failed for data set table, when restarts the job again, t1 would be rolled backed both in data set table and metadata table, so what's the problem here ?

nsivabalan · 2022-03-09T01:28:12Z

@danny0405 : here is the scenario.
Lets say multi-writer is enabled and hence rollbacks are lazy. there is a commit C5 which got committed to MDT, but crashed before committing to data table. and the user restarts the pipeline. due to multi-writer, there are more commits added, but rollback will be triggered lazily by cleaner. Lets say cleaner configs are very easy (say 100 commits). So, by this time, archival could clean up the partially failed commit.

danny0405 · 2022-03-09T02:15:57Z

@danny0405 : here is the scenario. Lets say multi-writer is enabled and hence rollbacks are lazy. there is a commit C5 which got committed to MDT, but crashed before committing to data table. and the user restarts the pipeline. due to multi-writer, there are more commits added, but rollback will be triggered lazily by cleaner. Lets say cleaner configs are very easy (say 100 commits). So, by this time, archival could clean up the partially failed commit.

Can we disable lazy cleaner for restarts/bootstrap then ? The lazy cleaning makes sense for normal commit but it just make things complex for boostrap/restarts and it even does not gains much.

yihua · 2022-03-10T07:51:07Z

@danny0405 : here is the scenario. Lets say multi-writer is enabled and hence rollbacks are lazy. there is a commit C5 which got committed to MDT, but crashed before committing to data table. and the user restarts the pipeline. due to multi-writer, there are more commits added, but rollback will be triggered lazily by cleaner. Lets say cleaner configs are very easy (say 100 commits). So, by this time, archival could clean up the partially failed commit.

Can we disable lazy cleaner for restarts/bootstrap then ? The lazy cleaning makes sense for normal commit but it just make things complex for boostrap/restarts and it even does not gains much.

For multi-writer scenario, we must have lazy cleaning since the job cannot tell if the inflight commit is due to failed write or actual inflight commit from another writer. So the job relies on the heartbeat timeout for determining the failed writes and lazily cleans up failed commits later. This is the whole point of having the guard we are discussing.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java

yihua

LGTM. Thanks for the critical fix!

danny0405 · 2022-03-24T12:15:01Z

@hudi-bot run azure

nsivabalan

thanks for the fix. LGTM

danny0405 · 2022-03-25T01:03:34Z

@hudi-bot run azure

…exist in metadata table active timeline

hudi-bot · 2022-03-26T03:41:46Z

CI report:

fdcd586 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

…exist in metadata table active timeline (apache#4821)

nsivabalan assigned yihua and nsivabalan Feb 16, 2022

nsivabalan added the priority:critical Production degraded; pipelines stalled label Feb 16, 2022

danny0405 force-pushed the HUDI-3435 branch 2 times, most recently from 1e329eb to b4f50a9 Compare February 18, 2022 03:02

danny0405 commented Feb 18, 2022

View reviewed changes

danny0405 force-pushed the HUDI-3435 branch from b4f50a9 to 1cc0107 Compare February 18, 2022 04:41

danny0405 force-pushed the HUDI-3435 branch from 1cc0107 to efbf8c7 Compare February 18, 2022 08:19

nsivabalan reviewed Feb 18, 2022

View reviewed changes

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java Show resolved Hide resolved

nsivabalan requested changes Feb 18, 2022

View reviewed changes

yihua reviewed Mar 4, 2022

View reviewed changes

yihua reviewed Mar 22, 2022

View reviewed changes

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java Show resolved Hide resolved

danny0405 force-pushed the HUDI-3435 branch from efbf8c7 to e2c21c3 Compare March 23, 2022 07:35

yihua approved these changes Mar 23, 2022

View reviewed changes

danny0405 force-pushed the HUDI-3435 branch from e2c21c3 to 47508af Compare March 24, 2022 10:23

danny0405 force-pushed the HUDI-3435 branch from 47508af to 7abbc6f Compare March 24, 2022 13:11

nsivabalan approved these changes Mar 24, 2022

View reviewed changes

yihua force-pushed the HUDI-3435 branch from 7abbc6f to ef96e99 Compare March 24, 2022 21:31

apache deleted a comment from hudi-bot Mar 24, 2022

yihua force-pushed the HUDI-3435 branch from ef96e99 to 101fcaa Compare March 25, 2022 00:33

apache deleted a comment from hudi-bot Mar 25, 2022

danny0405 force-pushed the HUDI-3435 branch 2 times, most recently from 5f5a782 to 2f1368d Compare March 26, 2022 01:00

[HUDI-3435] Do not throw exception when instant to rollback does not …

fdcd586

…exist in metadata table active timeline

danny0405 force-pushed the HUDI-3435 branch from 2f1368d to fdcd586 Compare March 26, 2022 01:05

danny0405 merged commit 0c09a97 into apache:master Mar 26, 2022

vingov pushed a commit to vingov/hudi that referenced this pull request Apr 3, 2022

[HUDI-3435] Do not throw exception when instant to rollback does not …

b5659ff

…exist in metadata table active timeline (apache#4821)

danny0405 mentioned this pull request May 25, 2022

[SUPPORT] Archive can't be triggered,when parameter of the metadata table was use in the program #5671

Closed

[HUDI-3435] Do not throw exception when instant to rollback does not … #4821

[HUDI-3435] Do not throw exception when instant to rollback does not … #4821

Uh oh!

Conversation

danny0405 commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

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

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danny0405 commented Feb 18, 2022

Uh oh!

danny0405 commented Feb 18, 2022

Uh oh!

danny0405 commented Feb 18, 2022

Uh oh!

nsivabalan 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!

nsivabalan left a comment

Choose a reason for hiding this comment

Uh oh!

danny0405 commented Feb 23, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsivabalan commented Mar 6, 2022

Uh oh!

danny0405 commented Mar 7, 2022

Uh oh!

nsivabalan commented Mar 9, 2022

Uh oh!

danny0405 commented Mar 9, 2022

Uh oh!

yihua commented Mar 10, 2022

Uh oh!

Uh oh!

yihua left a comment

Choose a reason for hiding this comment

Uh oh!

danny0405 commented Mar 24, 2022

Uh oh!

nsivabalan left a comment

Choose a reason for hiding this comment

Uh oh!

danny0405 commented Mar 25, 2022

Uh oh!

hudi-bot commented Mar 26, 2022

CI report:

Uh oh!

danny0405 commented Feb 15, 2022 •

edited

Loading