[HUDI-5279] move logic for deleting active instant to HoodieActiveTimeline by YannByron · Pull Request #7196 · apache/hudi

YannByron · 2022-11-14T10:12:16Z

Change Logs

Currently, only the logic for deleting active instant is out of HoodieActiveTimeline.
I think it's better that all the operations to active instant are concentrated to HoodieActiveTimeline.

Impact

none

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

none

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

YannByron · 2022-11-15T02:05:24Z

@hudi-bot run azure

danny0405 · 2022-11-15T07:39:50Z

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

-    Path inFlightCommitFilePath = getInstantFileNamePath(instant.getFileName());
-    try {
-      if (metaClient.getFs().exists(inFlightCommitFilePath)) {
-        boolean result = metaClient.getFs().delete(inFlightCommitFilePath, false);


Seems the core change is renaming a method here: deleteInstantFileIfExists -> deleteInstantIfExists ? If that is true, i would suggest to keep as it is.

no.
the major change is that, when need to delete active instant in archive operation, use HoodieActiveTimeline's deleteInstantIfExists instead of calling the naked api (fs.delete). After the changes, HoodieActiveTimeline becomes the standard route to operate active instants.

For this above, merge the origin method deleteInstantFile (used in current class and subclass) and deleteInstantFileIfExists (used outside) to one, and rename to deleteInstantIfExists.

calling the naked api (fs.delete).

deleteInstantFile(HoodieInstant instant) also takes a HoodieInstant param, so what's the difference here ?

the old way to delete active instants when archive uses deleteArchivedInstantFiles -> deleteFilesParallelize in HoodieTimelineArchiver. and deleteFilesParallelize calls fs.delete() directly.

danny0405 · 2022-11-15T08:41:14Z

hudi-common/src/main/java/org/apache/hudi/common/fs/FSUtils.java

+  public static <T, R> Map<T, R> parallelizeProcess(
+      HoodieEngineContext hoodieEngineContext,
+      int parallelism,
+      SerializableFunction<T, R> pairFunction,


Seems a general method for parallel processing, should not put it in FSUtils, maybe we can add a similar tool method as HoodieTimelineArchiver#deleteFilesParallelize for hoodie instants, for example HoodieTimelineArchiver#deleteInstantsParallelize

danny0405 · 2022-11-15T08:42:31Z

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

-        if (result) {
-          LOG.info("Removed instant " + instant);
-        } else {
-          throw new HoodieIOException("Could not delete instant " + instant);


The method save one invocation for fs.exists, let's keep it.

XuQianJin-Stars · 2022-11-24T23:19:56Z

@hudi-bot run azure

YannByron · 2022-11-25T02:11:57Z

@hudi-bot run azure

this CI has passed. @XuQianJin-Stars

xushiyan

a few notes:

at timeline API level, we should not allow ignoring error; this affects data integrity
there is another API org.apache.hudi.common.table.timeline.HoodieActiveTimeline#deleteInstantFile(org.apache.hudi.common.table.timeline.HoodieInstant) to be consolidated/dedup'ed
pls file jira as this touches critical code path, also properly fill the PR template "Impact" and "Risk" sections
pls help increase UT coverage for critical APIs wherever applicable

xushiyan · 2022-11-25T11:48:52Z

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

-      success &= result.getValue();
+    Map<HoodieInstant, Boolean> result = context.mapToPair(
+        instants,
+        instant -> ImmutablePair.of(


hide implementation: use Pair.of()

xushiyan · 2022-11-25T11:53:53Z

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

+    boolean success = true;
+    for (Map.Entry<HoodieInstant, Boolean> entry : result.entrySet()) {
+      LOG.info("Archived and deleted instant " + entry.getKey().toString() + " : " + entry.getValue());
+      success &= entry.getValue();


you can chain this with result using stream API allMatch()

if throw error directly if fail to delete, no return value is required.

xushiyan · 2022-11-25T11:56:21Z

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

+    deleteInstantFileIfExists(instant, true);
+  }
+
+  public boolean deleteInstantFileIfExists(HoodieInstant instant, boolean exceptionIfFailToDelete) {


Suggested change

public boolean deleteInstantFileIfExists(HoodieInstant instant, boolean exceptionIfFailToDelete) {

public boolean deleteInstantFileIfExists(HoodieInstant instant, boolean throwIfFailed) {

in what case do we want to silence the failure? this relates to timeline integrity so we should prefer to fail out loud

I think no cases will silence this. https://github.com/apache/hudi/pull/7196/files#r1032441151

xushiyan · 2022-11-25T12:12:04Z

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

-    Path inFlightCommitFilePath = getInstantFileNamePath(instant.getFileName());
+    Path commitFilePath = getInstantFileNamePath(instant.getFileName());


we need to be careful about what commit can be deleted. this API is designed to only delete requested/inflight or empty clean commit, based on the usage. (1 exception is in org.apache.hudi.cli.commands.TestRepairsCommand#testShowFailedCommits where this api is misused for deleting completed commits in tests; we should make separate test helper for that)

We can't delete completed commit instants, which breaks the timeline's integrity. So we should name it properly at the variable as well as the API level.

then we should add some doc for this method. inFlightCommitFilePath doesn't represent requested and empty clean instant.

xushiyan · 2022-11-25T12:29:15Z

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

+        instants,
+        instant -> ImmutablePair.of(
+            instant,
+            metaClient.getActiveTimeline().deleteInstantFileIfExists(instant, false)),


the original invocation is ignoreFailure=false; you inverted the flag here by saying exceptionIfFailToDelete=false, which silence the error

I was wrong about that. I thought the original logic was the ignoreFailure=false case. Will fix this.

YannByron · 2022-11-25T13:19:18Z

at timeline API level, we should not allow ignoring error; this affects data integrity

i thought there was a case that need to ignore this error. Will correct this.

there is another API org.apache.hudi.common.table.timeline.HoodieActiveTimeline#deleteInstantFile(org.apache.hudi.common.table.timeline.HoodieInstant) to be consolidated/dedup'ed

I know that and have did this. But Danny suggests that make this pr force on one question.

pls file jira as this touches critical code path, also properly fill the PR template "Impact" and "Risk" sections

file a jira. but after this pr is updated, there is almost no risk and impact.

pls help increase UT coverage for critical APIs wherever applicable

the existing UT is enough to cover this improvement.

xushiyan · 2022-11-27T04:41:40Z

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

+  private void deleteArchivedInstants(HoodieEngineContext context,
+                                      HoodieTableMetaClient metaClient,
+                                      List<HoodieInstant> instants) {
+    if (!instants.isEmpty()) {
+      context.foreach(
+          instants,
+          instant -> metaClient.getActiveTimeline().deleteInstantFileIfExists(instant),
+          Math.min(instants.size(), config.getArchiveDeleteParallelism())


no need to use a separate method: this method overload the other method, only signature changes that confuses people about which calls which. This is just 1 line invocation. you just call context.foreach() once for the 2 lists together by chaining them.

ok, not found there is another method with same name.

YannByron · 2022-11-29T02:16:25Z

@hudi-bot run azure

This reverts commit 4a760ca.

hudi-bot · 2022-11-30T09:48:17Z

CI report:

3b1e398 UNKNOWN
65c6fdd Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

…eline (apache#7196)

…eline (#7196)

…eline (apache#7196)

YannByron force-pushed the hudi_improve_atl branch from e9b0ac6 to eea310e Compare November 14, 2022 14:46

danny0405 reviewed Nov 15, 2022

View reviewed changes

YannByron force-pushed the hudi_improve_atl branch 2 times, most recently from 1145ecb to 47f4bd8 Compare November 16, 2022 03:12

apache deleted a comment from YannByron Nov 24, 2022

xushiyan requested changes Nov 25, 2022

View reviewed changes

YannByron changed the title ~~[MINOR] move logic for deleting active instant to HoodieActiveTimeline~~ [HUDI-5279] move logic for deleting active instant to HoodieActiveTimeline Nov 25, 2022

YannByron force-pushed the hudi_improve_atl branch from 47f4bd8 to 619effa Compare November 25, 2022 13:37

YannByron requested a review from xushiyan November 25, 2022 13:39

xushiyan reviewed Nov 27, 2022

View reviewed changes

YannByron force-pushed the hudi_improve_atl branch from 3b1e398 to 7f6d467 Compare November 28, 2022 02:52

YannByron requested a review from xushiyan November 28, 2022 02:52

xushiyan approved these changes Nov 28, 2022

View reviewed changes

YannByron added 4 commits November 29, 2022 14:14

[MINOR] move logic for deleting active instant to HoodieActiveTimeline

13c31f3

solve comments

d741194

update

4d6c6b8

update

a7d0518

YannByron force-pushed the hudi_improve_atl branch from 43684b3 to a7d0518 Compare November 29, 2022 06:14

YannByron added 2 commits November 29, 2022 22:16

test CI

4a760ca

Revert "test CI"

65c6fdd

This reverts commit 4a760ca.

YannByron merged commit fdce2b8 into apache:master Nov 30, 2022

vamshigv pushed a commit to vamshigv/hudi that referenced this pull request Dec 1, 2022

[HUDI-5279] move logic for deleting active instant to HoodieActiveTim…

b9144a3

…eline (apache#7196)

satishkotha pushed a commit that referenced this pull request Dec 13, 2022

[HUDI-5279] move logic for deleting active instant to HoodieActiveTim…

8d0fe3c

…eline (#7196)

nsivabalan pushed a commit that referenced this pull request Dec 13, 2022

[HUDI-5279] move logic for deleting active instant to HoodieActiveTim…

a0ab401

…eline (#7196)

fengjian428 pushed a commit to fengjian428/hudi that referenced this pull request Apr 5, 2023

[HUDI-5279] move logic for deleting active instant to HoodieActiveTim…

90fe38a

…eline (apache#7196)

	public boolean deleteInstantFileIfExists(HoodieInstant instant, boolean exceptionIfFailToDelete) {
	public boolean deleteInstantFileIfExists(HoodieInstant instant, boolean throwIfFailed) {

		Path inFlightCommitFilePath = getInstantFileNamePath(instant.getFileName());
		Path commitFilePath = getInstantFileNamePath(instant.getFileName());

Conversation

YannByron commented Nov 14, 2022

Change Logs

Impact

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

Documentation Update

Contributor's checklist

Uh oh!

YannByron commented Nov 15, 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!

XuQianJin-Stars commented Nov 24, 2022

Uh oh!

YannByron commented Nov 25, 2022

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YannByron Nov 25, 2022 • 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!

YannByron commented Nov 25, 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!

YannByron commented Nov 29, 2022

Uh oh!

hudi-bot commented Nov 30, 2022

CI report:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xushiyan left a comment •

edited

Loading

YannByron Nov 25, 2022 •

edited

Loading

YannByron commented Nov 25, 2022 •

edited

Loading