[HUDI-9438] Fix conflict handling for compaction instants for v8 tables #13347

the-other-tim-brown · 2025-05-22T20:45:57Z

Change Logs

Previously the log files were associated with the latest base file at the time the commit starts. Since compaction planning or a successful compaction causes a new base file, we need to check for conflicts with compaction for these older table versions.
Starting with table version 8, the log files are associated with a base file based on the completion time so we don't rely on the current state of a file group when creating these log files. That means we no longer need to check compaction for conflicts which in turn allows for smoother operations on MoR tables.

Impact

Allows async compaction to schedule and run without causing disruption to other writers

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

High, this is a core change to conflict detection.

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

the-other-tim-brown · 2025-05-22T23:27:15Z

...client/transaction/TestSimpleConcurrentFileWritesConflictResolutionStrategyWithMORTable.java

-  }
-
-  @Test
-  public void testConcurrentWritesWithInterleavingInflightCompaction() throws Exception {


Moved this to the other class to consolidate the tests

danny0405 · 2025-05-23T02:12:07Z

...org/apache/hudi/client/transaction/SimpleConcurrentFileWritesConflictResolutionStrategy.java

    // 3. Get any completed replace commit that happened since the last successful write and any pending replace commit.
    // We need to check for write conflicts since they may have mutated the same files that are being newly created by the current write.
+    Predicate<HoodieInstant> compactionInstantFilter = (HoodieInstant instant) -> {


Can we add a new strategy class and not mess up the v8 and v6.

Do you mean a new implementation of ConflictResolutionStrategy? It would allow the user to potentially specify the class that does not map to their table version which can cause issues.

One way to make the code cleaner is to have private methods for handling the different versions so this method just checks table version and then delegates to these methods. Then the javadocs per method can also contain the specifics for that table version.

I've split this out into two internal methods which seems like the best way to ensure users automatically pick up these improvements if they upgraded to table version 8. The diff is also smaller now so it is more clear what is changing as part of this PR.

TheR1sing3un · 2025-05-28T09:37:33Z

...org/apache/hudi/client/transaction/SimpleConcurrentFileWritesConflictResolutionStrategy.java

+    Stream<HoodieInstant> completedCommitsInstantStream = activeTimeline
+        .getCommitsTimeline()
+        .filterCompletedInstants()
+        .filter(instant -> !isMoRTable || !instant.getAction().equals(HoodieTimeline.COMMIT_ACTION))


How about introduce a new method: HoodieTimeline#getDataCommitsTime

for mor: return [DELTA_COMMIT, REPLACE_COMMIT]

for cow: return [COMMIT, REPLACE_COMMIT]

vinothchandar

Can we add a new strategy class and not mess up the v8 and v6.

we cannot add a new class, since this is a user facing config . +1 for separating code for v6 and v8 such that its easy to drop v6 later down the line.

vinothchandar

How about introduce a new method: HoodieTimeline#getDataCommitsTime

for mor: return [DELTA_COMMIT, REPLACE_COMMIT]

for cow: return [COMMIT, REPLACE_COMMIT]

I think this handling is specific to conflict resolution. so good to keep it at this layer..

vinothchandar · 2025-05-29T22:46:53Z

...org/apache/hudi/client/transaction/SimpleConcurrentFileWritesConflictResolutionStrategy.java

+        .getCommitsTimeline()
+        .filterCompletedInstants()
+        .filter(instant -> !isMoRTable || !instant.getAction().equals(HoodieTimeline.COMMIT_ACTION))
+        .findInstantsAfter(lastSuccessfulInstant.isPresent() ? lastSuccessfulInstant.get().requestedTime() : HoodieTimeline.INIT_INSTANT_TS)


lets use lastSuccessfulInstant.map().orElse() ?

Similar here. the only thing thats different is the extra filter.. so we could pull the rest to a private method shared by both paths. and do the extra filter.

vinothchandar · 2025-05-29T22:48:30Z

...org/apache/hudi/client/transaction/SimpleConcurrentFileWritesConflictResolutionStrategy.java

+        .findInstantsAfter(lastSuccessfulInstant.isPresent() ? lastSuccessfulInstant.get().requestedTime() : HoodieTimeline.INIT_INSTANT_TS)
+        .getInstantsAsStream();
+
+    Stream<HoodieInstant> clusteringAndReplaceCommitInstants = activeTimeline


lets pull this into a private method.. called from both places.. ?

okay realize the difference filterPendingReplaceOrClusteringTimeline vs filterPendingReplaceClusteringAndCompactionTimeline . So okay to leave it as is or try and consolidate so the filter differences are obvious

vinothchandar · 2025-05-29T23:02:13Z

...apache/hudi/client/transaction/TestSimpleConcurrentFileWritesConflictResolutionStrategy.java

+  @ParameterizedTest
+  @ValueSource(booleans = {false, true})
+  public void testConcurrentWritesWithInterleavingSuccessfulCompaction(boolean preTableVersion8) throws Exception {
+    initMetaClient(preTableVersion8, HoodieTableType.MERGE_ON_READ);


I guess there are only two scenarios, that are different. So Parameterizing the entire test class is an overkill

vinothchandar

I think its safe for v8+ writers to not conflict with scheduled or completed compactions.

Without multi-writer, v6 writers should n't be conflicting with completed compactions (it ll only conflict with scheduled compactions)... With multi-writer, v6 writers will conflict with both scheduled and completed compactions. (the hasConflict() just looks for file group id overlap)

Let me know if yours is different. if not, LGTM. Suggest getting another review given Danny has started already..

vinothchandar · 2025-05-29T23:09:47Z

...org/apache/hudi/client/transaction/SimpleConcurrentFileWritesConflictResolutionStrategy.java

+        .findInstantsAfter(lastSuccessfulInstant.isPresent() ? lastSuccessfulInstant.get().requestedTime() : HoodieTimeline.INIT_INSTANT_TS)
+        .getInstantsAsStream();
+
+    Stream<HoodieInstant> clusteringAndReplaceCommitInstants = activeTimeline


okay realize the difference filterPendingReplaceOrClusteringTimeline vs filterPendingReplaceClusteringAndCompactionTimeline . So okay to leave it as is or try and consolidate so the filter differences are obvious

the-other-tim-brown · 2025-05-29T23:36:49Z

I think its safe for v8+ writers to not conflict with scheduled or completed compactions.

Without multi-writer, v6 writers should n't be conflicting with completed compactions (it ll only conflict with scheduled compactions)... With multi-writer, v6 writers will conflict with both scheduled and completed compactions. (the hasConflict() just looks for file group id overlap)

Let me know if yours is different. if not, LGTM. Suggest getting another review given Danny has started already..

@vinothchandar that is my understanding as well

hudi-bot · 2025-05-30T03:02:19Z

CI report:

fe0d198 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

…es (apache#13347) * fix conflict handling for compaction given completion time changes * consolidate tests * split handling into two methods for ease of reading and debugging * extract common parts of the code

fix conflict handling for compaction given completion time changes

59dc766

github-actions bot added the size:S PR with lines of changes in (10, 100] label May 22, 2025

consolidate tests

0e84fcd

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

the-other-tim-brown commented May 22, 2025

View reviewed changes

the-other-tim-brown mentioned this pull request May 22, 2025

[HUDI-9388] Add compaction and clustering plan conflict detection logic #13316

Closed

4 tasks

danny0405 reviewed May 23, 2025

View reviewed changes

split handling into two methods for ease of reading and debugging

65eb9f9

TheR1sing3un reviewed May 28, 2025

View reviewed changes

vinothchandar self-assigned this May 28, 2025

vinothchandar reviewed May 29, 2025

View reviewed changes

vinothchandar approved these changes May 29, 2025

View reviewed changes

extract common parts of the code

fe0d198

danny0405 approved these changes May 30, 2025

View reviewed changes

vinothchandar merged commit 450a3c2 into apache:master May 30, 2025
58 checks passed

[HUDI-9438] Fix conflict handling for compaction instants for v8 tables #13347

[HUDI-9438] Fix conflict handling for compaction instants for v8 tables #13347

Uh oh!

Conversation

the-other-tim-brown commented May 22, 2025 • 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!

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!

vinothchandar left a comment

Choose a reason for hiding this comment

Uh oh!

vinothchandar left a comment

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!

vinothchandar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

the-other-tim-brown commented May 29, 2025

Uh oh!

hudi-bot commented May 30, 2025

CI report:

Uh oh!

Uh oh!

Uh oh!

the-other-tim-brown commented May 22, 2025 •

edited

Loading