[HUDI-5569] Maintain commit timeline even in case of long standing inflights #8783

suryaprasanna · 2023-05-22T21:55:46Z

Change Logs

Leaving inflight files in the timeline can cause various problems. In the past we have seen issues, when oldest inflight commit is the first commit in the timeline. In that case readers treated the files written by the oldest files as valid.
In the existing logic there are lot of checks for multiwriter case, pending compaction and replacecommit etc. This proposed change has a unified approach to address all of such cases.
The change is to keep the timeline intact by finding the greatest completed commit in the timeline that is lower than oldest inflight commit, this commit is called oldestCommitToRetain. And any instant that is greater than or equal to this commit value is not eligible for archival.

Impact

Instead of tracking the oldestInflight commit, now we are tracking the completed commit that is before this inflight commit.

Risk level (write none, low medium or high below)

Added unit tests to test the changes.

Documentation Update

This changes effects internal features required documentation is provided as part of the comments.

Contributor's checklist

Read through contributor's guide
Change Logs and Impact were stated clearly
Adequate tests were added if applicable
CI passed

danny0405 · 2023-05-23T08:34:50Z

@suryaprasanna Thanks for the contribution ~, can you elaborate a little more what are we want to improve for archiving?

suryaprasanna · 2023-05-26T07:03:31Z

@danny0405 Thanks for reviewing this PR. Added more context to the PR. Please take a look.

danny0405 · 2023-05-26T10:55:23Z

In the past we have seen issues, when oldest inflight commit is the first commit in the timeline. In that case readers treated the files written by the oldest files as valid

I'm wondering what instant the reader is holding to query the table, we have a check inside the fs view, only fileslice with the complete instant can be explosed to readers.

suryaprasanna · 2023-05-28T07:15:42Z

The bug where reader's are reading from inflight instant is resolved.

With this change, I am trying to consolidate the approach of handling inflight commits. Since, we are handling pending compaction and replacecommits separately and it could lead to various corner cases.
For example, when cleaner's earliestToRetain can cross the inflight instant and when this inflight file is committed, we will have different versions of the files left in the partitions. These kind of various cases can play out.
So, I was thinking we need to have similar logic for both archival and cleaner. So, if there is a inflight commit present then both the archival and cleaner cannot cross the largestCompletedInstant i.e. < oldestInflighiInstant.
We can say cleaner's earliestCommitToRetain similarly archival's max instant to archive cannot cross this commit.

That is why I am trying to keep largestCompletedInstant that is less than the oldestInflightinstant from getting archived.

danny0405 · 2023-05-30T02:41:05Z

when cleaner's earliestToRetain can cross the inflight instant

This could not happen.

nsivabalan · 2023-05-31T23:54:59Z

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

+            // oldestCommitToRetain is the highest completed commit instant that is less than the oldest inflight instant.
+            // By filter out any commit >= oldestCommitToRetain, we can ensure there are no gaps in the timeline
+            // when inflight commits are present.
+            return oldestCommitToRetain


this simplifies a lot :)

Yeah, it can fix #7738, although a little obscure to understand.

nsivabalan · 2023-06-01T00:11:50Z

hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/io/TestHoodieTimelineArchiver.java

    }
  }

+  @Test


can we add some java docs on what we are testing here.
simple illustration of timeline might also help

nsivabalan · 2023-06-01T00:12:58Z

hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/io/TestHoodieTimelineArchiver.java

+            + "after 101 and also to maintain timeline have at least one completed commit before pending commit");
+    assertTrue(timeline.containsInstant(new HoodieInstant(State.INFLIGHT, HoodieTimeline.REPLACE_COMMIT_ACTION, "101")),
+        "Inflight replacecommit must still be present");
+    assertTrue(timeline.containsInstant(new HoodieInstant(false, HoodieTimeline.COMMIT_ACTION, "102")),


can you declare a list of instants to expect in active timeline. and run it in a for loop rather than 1 line for each instant

Sure, refactored the code.

nsivabalan · 2023-06-01T00:13:57Z

hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/io/TestHoodieTimelineArchiver.java

+    assertTrue(timeline.containsInstant(new HoodieInstant(false, HoodieTimeline.COMMIT_ACTION, "102")),
+        "Instants greater than oldest pending commit must be present");
+    assertTrue(timeline.containsInstant(new HoodieInstant(State.REQUESTED, HoodieTimeline.COMPACTION_ACTION, "103")),
+        "Instants greater than oldest pending commit must be present");


nsivabalan · 2023-06-01T00:15:27Z

hudi-common/src/main/java/org/apache/hudi/common/util/ClusteringUtils.java

+          // not for replaced file groups. So, last clean instant can be considered as a lower bound, since
+          // the cleaner would have removed all the file groups until then. But there is a catch to this logic,
+          // while cleaner is running if there is a pending replacecommit then those files are not cleaned.
+          // TODO: This case has to be handled.


can you file a tracking ticket for this and link here

Created couple of tickets please review them.
https://issues.apache.org/jira/browse/HUDI-6351
https://issues.apache.org/jira/browse/HUDI-6352

danny0405 · 2023-06-05T04:00:18Z

@suryaprasanna Hi, can you update and fix the test failures?

Summary: Maintain commit timeline even in case of long standing inflights Test Plan: Will include unit tests. Reviewers: satishkotha, O955 Project Hoodie Project Reviewer: Add blocking reviewers Reviewed By: satishkotha, O955 Project Hoodie Project Reviewer: Add blocking reviewers JIRA Issues: HUDI-1259 Differential Revision: https://code.uberinternal.com/D6483879

suryaprasanna · 2023-06-10T19:39:39Z

@danny0405 My bad, noticed that a fix went in recently(Jan 2023) which will block the issue from happening.

hudi-bot · 2023-06-10T23:31:49Z

CI report:

0cce98b UNKNOWN
da86a92 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

danny0405

+1, I love this fix.

suryaprasanna changed the title ~~Archival enhaceements~~ Archival enhancements May 22, 2023

danny0405 self-assigned this May 23, 2023

danny0405 added area:table-service Table services metadata area:performance Performance optimizations priority:medium Moderate impact; usability gaps labels May 23, 2023

suryaprasanna force-pushed the archival-changes branch from 9dbf3aa to 0cce98b Compare May 26, 2023 06:33

suryaprasanna changed the title ~~Archival enhancements~~ Maintain commit timeline even in case of long standing inflights May 26, 2023

nsivabalan requested changes Jun 1, 2023

View reviewed changes

nsivabalan changed the title ~~Maintain commit timeline even in case of long standing inflights~~ [HUDI-5569] Maintain commit timeline even in case of long standing inflights Jun 1, 2023

nsivabalan mentioned this pull request Jun 1, 2023

[HUDI-5569] Fixing TableFileSystemView to detect early failed commits #7738

Closed

4 tasks

danny0405 added priority:critical Production degraded; pipelines stalled and removed priority:medium Moderate impact; usability gaps labels Jun 5, 2023

suryaprasanna added 2 commits June 10, 2023 09:50

Address review comments

da86a92

suryaprasanna force-pushed the archival-changes branch from 3b336af to da86a92 Compare June 10, 2023 19:40

danny0405 approved these changes Jun 11, 2023

View reviewed changes

nsivabalan approved these changes Jun 14, 2023

View reviewed changes

nsivabalan merged commit 95d8c6b into apache:master Jun 14, 2023

danny0405 mentioned this pull request Jun 14, 2023

[HUDI-6361] retain the commit before inflignt compaction to support schema Evolution #8938

Closed

4 tasks

danny0405 mentioned this pull request Jul 21, 2023

[HUDI-6574] Fix the problem that incremental clean cannot be executed when the earliest ActiveTimeline is a pending commit. #9243

Closed

4 tasks

zhuanshenbsj1 mentioned this pull request Jul 23, 2023

[HUDI-6581] Remove unnecessary validations in function getOldestInstantToRetainForClustering #9265

Merged

4 tasks

                   }
                 }
+                @Test

[HUDI-5569] Maintain commit timeline even in case of long standing inflights #8783

[HUDI-5569] Maintain commit timeline even in case of long standing inflights #8783

Uh oh!

Conversation

suryaprasanna commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Logs

Impact

Risk level (write none, low medium or high below)

Documentation Update

Contributor's checklist

Uh oh!

danny0405 commented May 23, 2023

Uh oh!

suryaprasanna commented May 26, 2023

Uh oh!

danny0405 commented May 26, 2023

Uh oh!

suryaprasanna commented May 28, 2023

Uh oh!

danny0405 commented May 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danny0405 Jun 1, 2023 • 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!

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 Jun 5, 2023

Uh oh!

suryaprasanna commented Jun 10, 2023

Uh oh!

hudi-bot commented Jun 10, 2023

CI report:

Uh oh!

danny0405 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

suryaprasanna commented May 22, 2023 •

edited

Loading

danny0405 Jun 1, 2023 •

edited

Loading