[HUDI-8590] fix: wrong file path for consistent-bucket-commit-marker-file #12344

TheR1sing3un · 2024-11-27T02:57:02Z

issue: #12338

Change Logs

wrong file path for consistent-bucket-commit-marker-file

Describe context and summary for this change. Highlight if any code was copied.

Impact

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

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

low
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. If not, put "none".

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

TheR1sing3un · 2024-11-27T02:59:53Z

@beyond1920 @danny0405 HI, I find a bug about incorrect path for the Consistent-Bucket-Commit-Marker-File. Please have a look. Thanks!

...udi-client-common/src/main/java/org/apache/hudi/index/bucket/ConsistentBucketIndexUtils.java

yuzhaojing · 2024-11-28T10:07:32Z

Hi, could u please describe what problems will be caused if this fix is not made? Will there be any issues regarding correctness?

TheR1sing3un · 2024-11-28T12:04:05Z

Hi, could u please describe what problems will be caused if this fix is not made? Will there be any issues regarding correctness?

Commit-marker-file will always be created in wrong file path.You never know that a certain version of hash metadata has been committed because the scan path is different. Therefore, it is necessary to go to the timeline each time and then resubmit this file, which will put a lot of pressure on the timeline. In the current code logic, if there is only one hash metadata that is not committed and it is still in pending state, the current code logic will return empty instead of the recently committed hash metadata. So I fix these two problems.

...udi-client-common/src/main/java/org/apache/hudi/index/bucket/ConsistentBucketIndexUtils.java

danny0405 · 2024-12-09T10:07:02Z

...udi-client-common/src/main/java/org/apache/hudi/index/bucket/ConsistentBucketIndexUtils.java

+      metaFiles.stream().filter(hashingMetaCommitFilePredicate)
+          .forEach(commitFile -> {
+            String instantTime = HoodieConsistentHashingMetadata.getTimestampFromFile(commitFile.getPath().getName());
+            if (!versionedHashMetadataFiles.containsKey(instantTime)) {


Can we just fix the metadata path and keep the other logic as it is so that it is more easier to review, if you wanna a refactoring to the code, let's fire another PR to address it.

Can we just fix the metadata path and keep the other logic as it is so that it is more easier to review, if you wanna a refactoring to the code, let's fire another PR to address it.

I originally intended to do this like u said, but found that fixing the metadata path code exposed the problem of the original code, assuming that there is now an inflight clustering, when loading metadata, according to the original code, it will return empty, which is not expected. Therefore, I fixed these two problems at the same time, and it was difficult to fix them separately, because the original unit-test would be failed.

Let's try to fix the issues one by one

Let's try to fix the issues one by one

Hi! I have updated this pr. This pr will only fix the problem with the wrong file path, and I will initiate a new pr after this pr is merged with the changes in refactoring metadata management.

Let's try to fix the issues one by one

As you said, after we merging the wrong path problem fixing pr, I raise a new pr to refactor consistent-hash-metadata management for better code readability and maintainability, lets keep reviewing at: #12512

TheR1sing3un · 2024-12-16T07:53:12Z

@danny0405 Hi! I have added some tests to verify the rationality of these changes, please review it again.

...nt/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java

1. wrong file path for consistent-bucket-commit-marker-file 2. return the max committed metadata file when no hash metadata file has been successfully fixed Signed-off-by: TheR1sing3un <[email protected]>

danny0405 · 2024-12-17T04:15:29Z

...udi-client-common/src/main/java/org/apache/hudi/index/bucket/ConsistentBucketIndexUtils.java

+        }).findFirst());
+        ValidationUtils.checkState(maxCommittedMetadataFileOpt.isPresent(),
+            () -> "Failed to find max committed metadata file but commit marker file exist with instant: " + maxCommittedMetadataFileOpt);
+        fixed.add(maxCommittedMetadataFileOpt.get());


Can we return the maxCommittedMetadataFileOpt directly in line 175 instead of adding it to the fixed list.

Can we return the maxCommittedMetadataFileOpt directly in line 175 instead of adding it to the fixed list.

Nice suggestion! I have updated it.

…f adding it to the fixed list. 1. return the `maxCommittedMetadataFileOpt` directly instead of adding it to the fixed list. Signed-off-by: TheR1sing3un <[email protected]>

hudi-bot · 2024-12-17T14:22:29Z

CI report:

efde0e3 UNKNOWN
34c6135 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

github-actions bot added size:S PR with lines of changes in (10, 100] size:M PR with lines of changes in (100, 300] and removed size:S PR with lines of changes in (10, 100] labels Nov 27, 2024

TheR1sing3un force-pushed the fix_wrong_consistent_commit_marker_file branch 2 times, most recently from efde0e3 to fb1ea61 Compare November 27, 2024 10:51

danny0405 reviewed Nov 28, 2024

View reviewed changes

...udi-client-common/src/main/java/org/apache/hudi/index/bucket/ConsistentBucketIndexUtils.java Outdated Show resolved Hide resolved

TheR1sing3un force-pushed the fix_wrong_consistent_commit_marker_file branch from fb1ea61 to 13c80d9 Compare December 9, 2024 08:47

TheR1sing3un requested a review from danny0405 December 9, 2024 08:52

github-actions bot added size:L PR with lines of changes in (300, 1000] and removed size:M PR with lines of changes in (100, 300] labels Dec 9, 2024

danny0405 reviewed Dec 9, 2024

View reviewed changes

...udi-client-common/src/main/java/org/apache/hudi/index/bucket/ConsistentBucketIndexUtils.java Outdated Show resolved Hide resolved

danny0405 reviewed Dec 9, 2024

View reviewed changes

TheR1sing3un requested a review from danny0405 December 9, 2024 10:40

TheR1sing3un force-pushed the fix_wrong_consistent_commit_marker_file branch from 13c80d9 to 5ad0dc2 Compare December 16, 2024 07:46

danny0405 reviewed Dec 16, 2024

View reviewed changes

...nt/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java Outdated Show resolved Hide resolved

fix: wrong file path for consistent-bucket-commit-marker-file

97df8a4

1. wrong file path for consistent-bucket-commit-marker-file 2. return the max committed metadata file when no hash metadata file has been successfully fixed Signed-off-by: TheR1sing3un <[email protected]>

TheR1sing3un force-pushed the fix_wrong_consistent_commit_marker_file branch from 5ad0dc2 to 97df8a4 Compare December 16, 2024 10:57

TheR1sing3un requested a review from danny0405 December 16, 2024 10:59

github-actions bot added size:S PR with lines of changes in (10, 100] and removed size:L PR with lines of changes in (300, 1000] labels Dec 16, 2024

danny0405 reviewed Dec 17, 2024

View reviewed changes

refactor: return the maxCommittedMetadataFileOpt directly instead o…

34c6135

…f adding it to the fixed list. 1. return the `maxCommittedMetadataFileOpt` directly instead of adding it to the fixed list. Signed-off-by: TheR1sing3un <[email protected]>

TheR1sing3un requested a review from danny0405 December 17, 2024 12:35

danny0405 approved these changes Dec 18, 2024

View reviewed changes

danny0405 merged commit a1411a6 into apache:master Dec 18, 2024
42 of 43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-8590] fix: wrong file path for consistent-bucket-commit-marker-file #12344

[HUDI-8590] fix: wrong file path for consistent-bucket-commit-marker-file #12344

TheR1sing3un commented Nov 27, 2024

TheR1sing3un commented Nov 27, 2024

yuzhaojing commented Nov 28, 2024

TheR1sing3un commented Nov 28, 2024

danny0405 Dec 9, 2024

TheR1sing3un Dec 9, 2024 •

edited

Loading

danny0405 Dec 16, 2024

TheR1sing3un Dec 16, 2024

TheR1sing3un Dec 18, 2024 •

edited

Loading

TheR1sing3un commented Dec 16, 2024

danny0405 Dec 17, 2024

TheR1sing3un Dec 17, 2024

hudi-bot commented Dec 17, 2024

[HUDI-8590] fix: wrong file path for consistent-bucket-commit-marker-file #12344

[HUDI-8590] fix: wrong file path for consistent-bucket-commit-marker-file #12344

Conversation

TheR1sing3un commented Nov 27, 2024

Change Logs

Impact

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

Documentation Update

Contributor's checklist

TheR1sing3un commented Nov 27, 2024

yuzhaojing commented Nov 28, 2024

TheR1sing3un commented Nov 28, 2024

danny0405 Dec 9, 2024

Choose a reason for hiding this comment

TheR1sing3un Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

danny0405 Dec 16, 2024

Choose a reason for hiding this comment

TheR1sing3un Dec 16, 2024

Choose a reason for hiding this comment

TheR1sing3un Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

TheR1sing3un commented Dec 16, 2024

danny0405 Dec 17, 2024

Choose a reason for hiding this comment

TheR1sing3un Dec 17, 2024

Choose a reason for hiding this comment

hudi-bot commented Dec 17, 2024

CI report:

TheR1sing3un Dec 9, 2024 •

edited

Loading

TheR1sing3un Dec 18, 2024 •

edited

Loading