[HUDI-5341] IncrementalCleaning consider clustering completed later #7405

zhuanshenbsj1 · 2022-12-08T03:00:47Z

Change Logs

refer to #7401 (close accidentally)

In some scenes, such as offline clustering or online sync clustering(Parallel), later plans could completed before previous plans. If later plan belong to the next partation, and cleaning speed catch up with it，Incremental Cleaning mode for getPartitionPathsForIncrementalCleaning would ignore previous plans ，eventually lead to duplicate data.

partation : day=2022-12-06/hour=10/minute_per_10=0 , day=2022-12-06/hour=10/minute_per_10=1

cleaning catch up with clustering belong to partation day=2022-12-06/hour=10/minute_per_10=1
log:

instant:

the file in previous clustering plan belong to partation 2022-12-06/hour=10/minute_per_10=0 won't clean forever. and it will cause duplicate data when the instants archived.

Impact

Describe any public API or user-facing feature change or any performance impact.

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

If medium or high, explain what verification was done to mitigate the risks.

Documentation Update

Describe any necessary documentation update if there is any new feature, config, or user-facing change

The config description must be updated if new configs are added or the default value of the configs are changed
Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
ticket number here and follow the instruction to make
changes to the website.

Contributor's checklist

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

zhuanshenbsj1 · 2022-12-08T03:12:57Z

@danny0405 How aboud add a check before archive , varify that every instant completed time should be earlier than the last completed clean-instant start clean time one by one.

danny0405 · 2022-12-20T07:18:32Z

...client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java

+        instant -> (HoodieTimeline.compareTimestamps(instant.getTimestamp(), HoodieTimeline.GREATER_THAN_OR_EQUALS,
+            cleanMetadata.getEarliestCommitToRetain())
+              || (instant.getMarkerFileAccessTimestamp().isPresent()
+                && (HoodieTimeline.compareTimestamps(instant.getMarkerFileAccessTimestamp().get(), HoodieTimeline.GREATER_THAN_OR_EQUALS,


There are many cases that the instant meta file can be accessed , not just cluetring.

There are many cases that the instant meta file can be accessed , not just cluetring.

Adjust access time to modification time.

danny0405 · 2022-12-20T07:19:53Z

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

+              earlestUnCleanCompletedInstant.map(instant ->
+                      compareTimestamps(instant.getTimestamp(), GREATER_THAN, s.getTimestamp()))
+                  .orElse(true)
+          ).filter(s ->


We better check the timeline clean metadata files, only when all the replaced clustering files are cleaned, can the clustering instant be archived.

We better check the timeline clean metadata files, only when all the replaced clustering files are cleaned, can the clustering instant be archived.

no completed clustering instant -> no need to check
firstCompletedClusteringInstant is not null && no completed clean instant -> return firstCompletedClusteringInstant
firstCompletedClusteringInstant is not null && earliestCleanInstant is not null -> return firstUncleanClusteringInstant

SteNicholas · 2022-12-27T02:42:53Z

hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieInstant.java

    this.state = isInflight ? State.INFLIGHT : State.COMPLETED;
    this.action = action;
    this.timestamp = timestamp;
+    this.markerFileModificationTimestamp = Option.ofNullable(null);


Removes this line.

Removes this line.

Removed.

SteNicholas · 2022-12-27T02:43:01Z

hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieInstant.java

    this.state = state;
    this.action = action;
    this.timestamp = timestamp;
+    this.markerFileModificationTimestamp = Option.ofNullable(null);


danny0405 · 2022-12-27T03:45:35Z

5341.patch.zip
Hi, @zhuanshenbsj1 , i have tried to review and applied a patch, add a weak guarantee on the archiver pre condition check, do you think that is okey ? It would be costy of we resolve all the clean metadata to see whether the clustering replaced files are all been cleaned.

We can go ahead if you think it is okey, and it would be great if you can test it also.

SteNicholas · 2022-12-27T06:46:04Z

...client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java

-            cleanMetadata.getEarliestCommitToRetain()) && HoodieTimeline.compareTimestamps(instant.getTimestamp(),
+        instant -> (HoodieTimeline.compareTimestamps(instant.getTimestamp(), HoodieTimeline.GREATER_THAN_OR_EQUALS,
+            cleanMetadata.getEarliestCommitToRetain())
+              || (instant.getMarkerFileModificationTimestamp().isPresent()


I didn't think the condition is needed for the commits which isn't the replacecommit.

Guarantee compatibility. Maybe other new kinds of instant need to clean later ?

If an out-of-order replace commit finished before the clean start and the instant time of the replace commit is before the earliest commit to retain, it won't be cleaned and left in the timeline. Archiver will then archive it since it's last modified time is earlier than the last clean in the timeline. What do you think?

If an out-of-order replace commit finished before the clean start and the instant time of the replace commit is before the earliest commit to retain, it won't be cleaned and left in the timeline. Archiver will then archive it since it's last modified time is earlier than the last clean in the timeline. What do you think?

You are right，it still won't clean the clustering instant in this scenario.

SteNicholas · 2022-12-27T07:27:42Z

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

+    HoodieActiveTimeline activeTimeline =  metaClient.getActiveTimeline();
+    Option<HoodieInstant> firstCompletedClusteringInstant = activeTimeline.getCommitsTimeline().filterCompletedInstants()
+        .filter(hoodieInstant -> hoodieInstant.getAction().equals(HoodieTimeline.REPLACE_COMMIT_ACTION)).firstInstant();
+    if (!firstCompletedClusteringInstant.isPresent()) {


If the firstCompletedClusteringInstant isn't present, it's no need to get the latest clean time from the clean metadata.

Move it behind else.

zhuanshenbsj1 · 2022-12-27T08:13:24Z

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

              table.getActiveTimeline(), config.getInlineCompactDeltaCommitMax())
              : Option.empty();

+      Option<HoodieInstant> earlestUnCleanCompletedInstant = CleanerUtils.getEarliestUnCleanCompletedInstant(metaClient);


zhuanshenbsj1 · 2022-12-27T08:20:11Z

5341.patch.zip Hi, @zhuanshenbsj1 , i have tried to review and applied a patch, add a weak guarantee on the archiver pre condition check, do you think that is okey ? It would be costy of we resolve all the clean metadata to see whether the clustering replaced files are all been cleaned.

We can go ahead if you think it is okey, and it would be great if you can test it also.

But this patch still not consider clustering completed time ? If previous clustering instant in timeline compelted later than following clean instant cleaned cutoff time， there are still risks.

hudi-bot · 2022-12-27T11:20:37Z

CI report:

cea266d Azure: FAILURE

Bot commands

@hudi-bot supports the following commands:

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

SteNicholas · 2022-12-27T11:58:18Z

@danny0405, @zhuanshenbsj1, @stream2000, IMO, the clean operation should modify the getEarliestCommitToRetain to keep the linear cleaning with the following:

if (config.getCleanerPolicy() == HoodieCleaningPolicy.KEEP_LATEST_COMMITS
        && commitTimeline.countInstants() > commitsRetained) {
      Option<HoodieInstant> earliestPendingCommits = hoodieTable.getMetaClient()
          .getActiveTimeline()
          .getCommitsTimeline()
          .filter(s -> !s.isCompleted()).firstInstant();
      if (earliestPendingCommits.isPresent()) {
        earliestCommitToRetain =
            commitTimeline.nthInstant(commitTimeline.countInstants() - commitsRetained).map(nthInstant -> {
              if (nthInstant.compareTo(earliestPendingCommits.get()) <= 0) {
                return Option.of(nthInstant);
              } else {
                return commitTimeline.findInstantsBefore(earliestPendingCommits.get().getTimestamp()).lastInstant();
              }
            }).orElse(Option.empty());
      } else {
        earliestCommitToRetain = commitTimeline.nthInstant(commitTimeline.countInstants() - commitsRetained); //15 instants total, 10 commits to retain, this gives 6th instant in the list
      }
 }

WDYT?

zhuanshenbsj1 · 2022-12-27T12:24:01Z

@danny0405, @zhuanshenbsj1, @stream2000, IMO, the clean operation should modify the getEarliestCommitToRetain to keep the linear cleaning with the following:

if (config.getCleanerPolicy() == HoodieCleaningPolicy.KEEP_LATEST_COMMITS
        && commitTimeline.countInstants() > commitsRetained) {
      Option<HoodieInstant> earliestPendingCommits = hoodieTable.getMetaClient()
          .getActiveTimeline()
          .getCommitsTimeline()
          .filter(s -> !s.isCompleted()).firstInstant();
      if (earliestPendingCommits.isPresent()) {
        earliestCommitToRetain =
            commitTimeline.nthInstant(commitTimeline.countInstants() - commitsRetained).map(nthInstant -> {
              if (nthInstant.compareTo(earliestPendingCommits.get()) <= 0) {
                return Option.of(nthInstant);
              } else {
                return commitTimeline.findInstantsBefore(earliestPendingCommits.get().getTimestamp()).lastInstant();
              }
            }).orElse(Option.empty());
      } else {
        earliestCommitToRetain = commitTimeline.nthInstant(commitTimeline.countInstants() - commitsRetained); //15 instants total, 10 commits to retain, this gives 6th instant in the list
      }
 }

WDYT?

It still need to confirm whether clustering instant has been cleaned when archiving.

SteNicholas · 2022-12-27T13:18:53Z

@zhuanshenbsj1, +1. I have modified the getOldestInstantToRetainForClustering of the above patch with the following to take the case you mentioned:

/**
   * Checks whether the latest clustering instant has a subsequent cleaning action. Returns
   * the clustering instant if there is such cleaning action or empty.
   *
   * @param activeTimeline The active timeline
   * @return the oldest instant to retain for clustering
   */
  public static Option<HoodieInstant> getOldestInstantToRetainForClustering(HoodieActiveTimeline activeTimeline)
      throws IOException {
    Option<HoodieInstant> cleanInstantOpt =
        activeTimeline.getCleanerTimeline().filter(instant -> !instant.isCompleted()).firstInstant();
    if (cleanInstantOpt.isPresent()) {
      // The first clustering instant of which timestamp is greater than or equal to the earliest commit to retain of
      // the clean metadata.
      HoodieInstant cleanInstant = cleanInstantOpt.get();
      String earliestCommitToRetain =
          TimelineMetadataUtils.deserializeHoodieCleanMetadata(
                  activeTimeline.getInstantDetails(cleanInstant.isRequested()
                      ? cleanInstant
                      : HoodieTimeline.getCleanRequestedInstant(cleanInstant.getTimestamp())).get())
              .getEarliestCommitToRetain();
      return activeTimeline.getCompletedReplaceTimeline()
          .filter(instant ->
              HoodieTimeline.compareTimestamps(
                  instant.getTimestamp(),
                  HoodieTimeline.GREATER_THAN_OR_EQUALS,
                  earliestCommitToRetain))
          .firstInstant();
    }
    return Option.empty();
  }

PTAL.

SteNicholas · 2023-01-06T05:47:43Z

@danny0405, IMO, this pull request could be close because the 7568 could replace the pull request.

leesf · 2023-01-06T08:33:27Z

closing the PR as we have the PR #7568

zhuanshenbsj1 added 2 commits December 7, 2022 13:37

IncrementalCleaning consider later clustering

e52b948

fix checkstyle error

ff0887a

zhuanshenbsj1 mentioned this pull request Dec 8, 2022

[HUDI-5341] IncrementalCleaning consider later clustering #7401

Closed

4 tasks

zhuanshenbsj1 added 2 commits December 16, 2022 01:09

add archive commit for clean to clustering

ff02d94

add archive commit for clean to clustering +1

b7a1071

danny0405 reviewed Dec 20, 2022

View reviewed changes

zhuanshenbsj1 added 2 commits December 27, 2022 01:17

change accessTime to modificationTime

1fb15d7

no need to get lastCompletedClusteringInstant

6779912

SteNicholas reviewed Dec 27, 2022

View reviewed changes

Adjust getCleantime: cleanInstant -> completed cleanInstant

cea266d

zhuanshenbsj1 commented Dec 27, 2022

View reviewed changes

SteNicholas mentioned this pull request Dec 28, 2022

[HUDI-5341] CleanPlanner retains earliest commits must not be later than earliest pending commit #7568

Merged

4 tasks

leesf closed this Jan 6, 2023

zhuanshenbsj1 deleted the clean_later_completed_clustering_1 branch January 8, 2024 07:42

[HUDI-5341] IncrementalCleaning consider clustering completed later #7405

[HUDI-5341] IncrementalCleaning consider clustering completed later #7405

Uh oh!

Conversation

zhuanshenbsj1 commented Dec 8, 2022

Change Logs

Impact

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

Documentation Update

Contributor's checklist

Uh oh!

zhuanshenbsj1 commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Dec 27, 2022

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!

zhuanshenbsj1 commented Dec 27, 2022

Uh oh!

hudi-bot commented Dec 27, 2022

CI report:

Uh oh!

SteNicholas commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhuanshenbsj1 commented Dec 27, 2022

Uh oh!

SteNicholas commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteNicholas commented Jan 6, 2023

Uh oh!

leesf commented Jan 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zhuanshenbsj1 commented Dec 8, 2022 •

edited

Loading

SteNicholas commented Dec 27, 2022 •

edited

Loading

SteNicholas commented Dec 27, 2022 •

edited

Loading