Skip to content

[HUDI-7207] Sequentially delete complete instant files in archival to prevent inconsistency during data reads #10325

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yihua merged 2 commits into from
Dec 14, 2023
Merged

[HUDI-7207] Sequentially delete complete instant files in archival to prevent inconsistency during data reads #10325

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7992915
[HUDI-7207] During archiving, complete instants are deleted sequentia…
majian1998 Dec 14, 2023
6dbe601
[HUDI-7207] During archiving, complete instants are deleted sequentia…
majian1998 Dec 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions ...i-client-common/src/main/java/org/apache/hudi/client/timeline/HoodieTimelineArchiver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,11 +342,13 @@ private boolean deleteArchivedInstants(List<ActiveAction> activeActions, HoodieE
);
}
if (!completedInstants.isEmpty()) {
context.foreach(
completedInstants,
instant -> activeTimeline.deleteInstantFileIfExists(instant),
Math.min(completedInstants.size(), config.getArchiveDeleteParallelism())
);
// Due to the concurrency between deleting completed instants and reading data,
// there may be hole in the timeline, which can lead to errors when reading data.
// Therefore, the concurrency of deleting completed instants is temporarily disabled,
// and instants are deleted in ascending order to prevent the occurrence of such holes.
// See HUDI-7207 and #10325.
completedInstants.stream()
.forEach(instant -> activeTimeline.deleteInstantFileIfExists(instant));
}
Copy link
Contributor

@danny0405 danny0405 Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the archived instants should be old, do you encouter data loss for table in production?

Copy link
Contributor Author

@majian1998 majian1998 Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I encountered this issue in an older version of Hudi, but I believe that the current version of Hudi has not addressed this problem.
As the example illustrates, let's assume that the order of ArchiveToInstant after sorting is 1, 2, 3, 4. Given a concurrency level of 2, instants 1 and 2 will be processed on one thread, while instants 3 and 4 will be processed on another. Suppose that the deletion of instant 1 is slow, and instants 3 and 4 are deleted first. This situation leads to the creation of an 'instant hole' in the timeline. If a query retrieves the timeline at this point, according to the rules for determining the visibility of the timeline, the files corresponding to instants 3 and 4 would be considered invisible.

Copy link
Contributor

@danny0405 danny0405 Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before release 1.0, Hudi relies on the existing file naming for file slicing. As long as there is parquet in the file group, the log would finally use the parquet instant time as its base instant time. The file slicing is not dependent to the commit metadata, the archiving sequence should not affect the file slicing version.

Since release 1.0, we use completion time file slicing, and the removing sequence does not matter because the completiom time of log is deterministic.

Copy link
Contributor

@danny0405 danny0405 Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, guess you are talking about this line:

hudi/hudi-common/src/main/java/org/apache/hudi/common/model/HoodieFileGroup.java

Line 153 in dd31c9b

if (!compareTimestamps(slice.getBaseInstantTime(), LESSER_THAN_OR_EQUALS, lastInstant.get().getTimestamp())) {

Copy link
Contributor

@yihua yihua Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@majian1998 This is a good find. The issue you mentioned is legit, though it can only happen during the parallelized deletion of instant files in the active timeline.

@danny0405 yeah, you pointed the right method. The file group instance uses the active timeline loaded to determine if a file slice is committed, if either is true: (1) the active timeline contains the instant time, or (2) the instant time is smaller than the start of the active timeline.

  /**
   * A FileSlice is considered committed, if one of the following is true - There is a committed data file - There are
   * some log files, that are based off a commit or delta commit.
   */
  private boolean isFileSliceCommitted(FileSlice slice) {
    if (!compareTimestamps(slice.getBaseInstantTime(), LESSER_THAN_OR_EQUALS, lastInstant.get().getTimestamp())) {
      return false;
    }

    return timeline.containsOrBeforeTimelineStarts(slice.getBaseInstantTime());

In the case @majian1998 mentions, i.e., 3.deltacommit is deleted first from the active timeline 1.deltacommit, 2.deltacommit, and 4.deltacommit, the data files written by 3.deltacommit are considered non-committed in such a transient state, since neither condition of the above is true.

So, the instant files have to be deleted in order based on the instant time (not the case as of master). Within the same instant time, the files should be deleted in the order of REQUESTED, INFLIGHT, COMPLETED (<ts>.commit etc. should be deleted last, which is the case now).

After looking at the history, the deletion did happen in order before until #3920 introduces the parallelized deletion. We need to revert that. In normal cases where archival runs for every commit, usually a handful of instant files are deleted so the performance hit may not be noticeable.

Copy link
Contributor

@yihua yihua Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A side note: Hudi makes sure the inflight data files are cleaned before archival happens on the relevant instants. So the base files with the instant time before the start of the active timeline are always committed, thus the check logic in isFileSliceCommitted(FileSlice).

Copy link
Contributor Author

@majian1998 majian1998 Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! @yihua You got it! Haha. Just as @danny0405 pointed out, that piece of code isFileSliceCommitted(FileSlice) will have issues during concurrent deletion of completed instants. The order of deletion doesn't matter for inflight ones.

Copy link
Contributor Author

@majian1998 majian1998 Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I believe that we just need to serialize the deletion of completed instants in order right here, while we can still maintain the concurrency of deleting inflight ones. cc @danny0405 @yihua

Copy link
Contributor

@yihua yihua Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. @majian1998 do you want to take a stab to fix the issue in this PR?


return true;
Expand Down