[HUDI-9158] Add storage-based lock provider abstract implementation #13103

alexr17 · 2025-04-08T03:44:22Z

Change Logs

Adds the abstract implementation of the storage-based lock provider using conditional writes.

See RFC: #12927

Previous PRs: #12958, #12954

Impact

Adds abstract lock provider implementation

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

None, without StorageLock implementation, this class will not be used.

Documentation Update

Config updates added here: #13104

Contributor's checklist

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

alexr17 · 2025-04-08T15:16:47Z

@hudi-bot run azure

...mmon/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockProvider.java

...common/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockConfig.java

...mmon/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockProvider.java

yihua · 2025-04-08T21:46:21Z

...mmon/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockProvider.java

+            lockFilePath,
+            Thread.currentThread(),
+            ACQUIRING);
+        Thread.sleep(DEFAULT_LOCK_ACQUISITION_BUFFER);


LockManager#lock already has retry logic when calling LockProvider#tryLock. Should the retry logic here be removed?

We are supposed to tryLock for the appropriate amount of time. I think the implementations in Zookeeper and Dynamo follow the same pattern here

We are supposed to tryLock for the appropriate amount of time

This makes sense, but it does not mean that we should retry at this layer.

...mmon/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockProvider.java

...on/src/test/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockConfigTest.java

hudi-io/src/main/java/org/apache/hudi/storage/StorageSchemes.java

hudi-hadoop-common/src/test/java/org/apache/hudi/common/fs/TestStorageSchemes.java

alexr17 · 2025-04-09T16:26:45Z

@hudi-bot run azure

alexr17 · 2025-04-09T23:21:27Z

@hudi-bot run azure

...ent-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockConfig.java

danny0405 · 2025-04-10T00:52:41Z

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java

@@ -125,13 +119,12 @@ private synchronized void setLock(StorageLockFile lockObj) {
   *                          ConditionalWriteLockConfig
   * @param conf              Storage config, ignored.
   */
-  public ConditionalWriteLockProvider(final LockConfiguration lockConfiguration, final StorageConfiguration<?> conf) {
-    ConditionalWriteLockConfig config = new ConditionalWriteLockConfig.Builder()


FIx the doc too for the renaming.

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java

danny0405 · 2025-04-10T01:08:25Z

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java

@@ -276,9 +264,9 @@ private synchronized boolean isLockStillValid(StorageLockFile lock) {
   */
  @Override
  public synchronized boolean tryLock() {
-    assertHeartBeatManagerExists();
+    assertHeartbeatManagerExists();


maybe we just use ValidationUtils.checkState instead

I'm not familiar with what this does?

I guess this could work, but I think we want to throw HoodieLockException, so not sure if IllegalStateException makes sense.

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java

...common/src/test/java/org/apache/hudi/client/transaction/lock/StorageBasedLockConfigTest.java

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/StorageBasedLockConfig.java

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java

nsivabalan · 2025-04-10T23:29:36Z

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java

+    // It does not make sense to have heartbeat alive extending the lock lease while
+    // here we are trying
+    // to expire the lock.
+    if (heartbeatManager.hasActiveHeartbeat()) {


please do remember that shutdown hook can get called even in the event of graceful shutdown.
So, these methods should be idempotent when called again.

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java

hudi-hadoop-common/src/test/java/org/apache/hudi/common/fs/TestStorageSchemes.java

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/StorageBasedLockConfig.java

danny0405

+1, LGTM.

yihua

Blocked on my final review

nsivabalan

minor nits. LGTM.

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java

hudi-hadoop-common/src/test/java/org/apache/hudi/common/fs/TestStorageSchemes.java

…lient/transaction/lock/ConditionalWriteLockConfig.java Co-authored-by: Y Ethan Guo <ethan.guoyihua@gmail.com>

…lient/transaction/lock/ConditionalWriteLockProvider.java Co-authored-by: Y Ethan Guo <ethan.guoyihua@gmail.com>

Co-authored-by: Y Ethan Guo <ethan.guoyihua@gmail.com>

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/StorageBasedLockConfig.java

yihua · 2025-04-11T06:22:01Z

...mmon/src/test/java/org/apache/hudi/client/transaction/lock/TestStorageBasedLockProvider.java

+    when(mockLockService.tryCreateOrUpdateLockFileWithRetry(any(), eq(nearExpiredLockFile), anyLong()))
+        .thenReturn(Pair.of(LockUpdateResult.ACQUIRED_BY_OTHERS, null));
+    assertFalse(lockProvider.renewLock());
+    verify(mockLogger).error("Owner {}: Unable to renew lock as it is acquired by others.", this.ownerId);


nit: checking logger is not the best way to validate the logic. If it's not easy to validate without this way, we should think about improving the interface in the future for testing. HUDI-9307 to track.

For this one, it's just an additional check. The assert false above is sufficient for testing this code path.

I think we have a lot of different logger statements for the same return value so I actually think this is one scenario where validating this way makes sense.

...mmon/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockProvider.java

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java

yihua · 2025-04-11T06:36:06Z

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java

+          // If the next heartbeat run identifies our lock has expired we will error out.
+          logger.warn("Owner {}: Unable to renew lock due to unknown error, could be transient.", ownerId);
+          // Let heartbeat retry later.
+          return true;


Should this return false based on the docs?

Docs should be updated. We should always retry renewal unless we receive a known fatal error code.

yihua · 2025-04-11T06:47:20Z

@alexr17 please update the RFC based on the latest code, including the configs, retries, shutdown hooks, etc., so that the RFC is consistent with the implementation.

yihua

LGTM overall. Please address remaining nits in a follow-up PR so we can land this PR to unblock stacked PRs on top of it.

hudi-bot · 2025-04-11T08:17:17Z

CI report:

b0d95ce UNKNOWN
d403ae4 UNKNOWN
6f63e44 UNKNOWN
fd6e8a8 UNKNOWN
88184cd UNKNOWN
b917d9f UNKNOWN
2d596de UNKNOWN
03f4a6c UNKNOWN
8f21de8 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

…pache#13103) * Add initial changes for storage-based lock provider abstract implementation --------- Co-authored-by: Y Ethan Guo <ethan.guoyihua@gmail.com> (cherry picked from commit 3dc2df5)

github-actions bot added the size:XL PR with lines of changes > 1000 label Apr 8, 2025

alexr17 mentioned this pull request Apr 8, 2025

[HUDI-9158] Add doc updates for conditional writes-based lock provider #13104

Merged

4 tasks

yihua reviewed Apr 8, 2025

View reviewed changes

...mmon/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockProvider.java Outdated Show resolved Hide resolved

yihua reviewed Apr 8, 2025

View reviewed changes

...common/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockConfig.java Outdated Show resolved Hide resolved

...common/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockConfig.java Outdated Show resolved Hide resolved

yihua reviewed Apr 8, 2025

View reviewed changes

...mmon/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockProvider.java Outdated Show resolved Hide resolved

...mmon/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockProvider.java Outdated Show resolved Hide resolved

yihua reviewed Apr 8, 2025

View reviewed changes

...mmon/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockProvider.java Outdated Show resolved Hide resolved

...mmon/src/main/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockProvider.java Outdated Show resolved Hide resolved

yihua reviewed Apr 8, 2025

View reviewed changes

...on/src/test/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockConfigTest.java Outdated Show resolved Hide resolved

...on/src/test/java/org/apache/hudi/client/transaction/lock/ConditionalWriteLockConfigTest.java Outdated Show resolved Hide resolved

yihua reviewed Apr 8, 2025

View reviewed changes

hudi-io/src/main/java/org/apache/hudi/storage/StorageSchemes.java Outdated Show resolved Hide resolved

hudi-hadoop-common/src/test/java/org/apache/hudi/common/fs/TestStorageSchemes.java Show resolved Hide resolved

danny0405 reviewed Apr 10, 2025

View reviewed changes

...ent-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockConfig.java Outdated Show resolved Hide resolved

danny0405 reviewed Apr 10, 2025

View reviewed changes

...ent-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockConfig.java Outdated Show resolved Hide resolved

danny0405 reviewed Apr 10, 2025

View reviewed changes

...ent-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockConfig.java Outdated Show resolved Hide resolved

danny0405 reviewed Apr 10, 2025

View reviewed changes

...ent-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockConfig.java Outdated Show resolved Hide resolved

danny0405 reviewed Apr 10, 2025

View reviewed changes

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java Show resolved Hide resolved

danny0405 reviewed Apr 10, 2025

View reviewed changes

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java Show resolved Hide resolved

danny0405 reviewed Apr 10, 2025

View reviewed changes

...common/src/test/java/org/apache/hudi/client/transaction/lock/StorageBasedLockConfigTest.java Outdated Show resolved Hide resolved

nsivabalan reviewed Apr 10, 2025

View reviewed changes

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/StorageBasedLockConfig.java Outdated Show resolved Hide resolved

danny0405 approved these changes Apr 10, 2025

View reviewed changes

yihua requested changes Apr 11, 2025

View reviewed changes

nsivabalan reviewed Apr 11, 2025

View reviewed changes

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java Outdated Show resolved Hide resolved

hudi-hadoop-common/src/test/java/org/apache/hudi/common/fs/TestStorageSchemes.java Outdated Show resolved Hide resolved

alexr17 and others added 16 commits April 10, 2025 23:14

Update hudi-client/hudi-client-common/src/main/java/org/apache/hudi/c…

39d9430

…lient/transaction/lock/ConditionalWriteLockConfig.java Co-authored-by: Y Ethan Guo <ethan.guoyihua@gmail.com>

Update hudi-client/hudi-client-common/src/main/java/org/apache/hudi/c…

a465892

…lient/transaction/lock/ConditionalWriteLockProvider.java Co-authored-by: Y Ethan Guo <ethan.guoyihua@gmail.com>

Update hudi-client/hudi-client-common/src/main/java/org/apache/hudi/c…

bf66cfc

…lient/transaction/lock/ConditionalWriteLockProvider.java Co-authored-by: Y Ethan Guo <ethan.guoyihua@gmail.com>

Update hudi-client/hudi-client-common/src/main/java/org/apache/hudi/c…

6d67370

…lient/transaction/lock/ConditionalWriteLockProvider.java Co-authored-by: Y Ethan Guo <ethan.guoyihua@gmail.com>

Apply suggestions from code review

95266ea

Co-authored-by: Y Ethan Guo <ethan.guoyihua@gmail.com>

respond to feedback and refactor models

fdd7113

update config

5519af2

rename class

6954801

conditional writes rename

d5310ed

add shutdown hook

e081f83

refactor configs based on feedback

2d3784b

cleanup lp initialization based on feedback

45d1b48

checkstyle

188735a

respond to minor feedback

06d4900

convert to milliseconds

269c57d

Address comments

2d596de

yihua force-pushed the HUDI-9158/base-lp branch from 0a1e4ac to 2d596de Compare April 11, 2025 06:24

yihua reviewed Apr 11, 2025

View reviewed changes

Address more comment

03f4a6c

yihua reviewed Apr 11, 2025

View reviewed changes

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java Outdated Show resolved Hide resolved

...t-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java Outdated Show resolved Hide resolved

yihua reviewed Apr 11, 2025

View reviewed changes

Address nits

8f21de8

yihua approved these changes Apr 11, 2025

View reviewed changes

danny0405 merged commit 3dc2df5 into apache:master Apr 11, 2025
60 checks passed

voonhous added the release-1.0.2 label Apr 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-9158] Add storage-based lock provider abstract implementation #13103

[HUDI-9158] Add storage-based lock provider abstract implementation #13103

alexr17 commented Apr 8, 2025 •

edited

Loading

alexr17 commented Apr 8, 2025

yihua Apr 8, 2025

alexr17 Apr 8, 2025

yihua Apr 11, 2025 •

edited

Loading

alexr17 commented Apr 9, 2025

alexr17 commented Apr 9, 2025

danny0405 Apr 10, 2025

danny0405 Apr 10, 2025

alexr17 Apr 10, 2025

alexr17 Apr 10, 2025

nsivabalan Apr 10, 2025

danny0405 left a comment

yihua left a comment

nsivabalan left a comment

yihua Apr 11, 2025

alexr17 Apr 11, 2025

yihua Apr 11, 2025

alexr17 Apr 11, 2025

yihua commented Apr 11, 2025

yihua left a comment

hudi-bot commented Apr 11, 2025

[HUDI-9158] Add storage-based lock provider abstract implementation #13103

[HUDI-9158] Add storage-based lock provider abstract implementation #13103

Conversation

alexr17 commented Apr 8, 2025 • edited Loading

Change Logs

Impact

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

Documentation Update

Contributor's checklist

alexr17 commented Apr 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihua Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

alexr17 commented Apr 9, 2025

alexr17 commented Apr 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danny0405 left a comment

Choose a reason for hiding this comment

yihua left a comment

Choose a reason for hiding this comment

nsivabalan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihua commented Apr 11, 2025

yihua left a comment

Choose a reason for hiding this comment

hudi-bot commented Apr 11, 2025

CI report:

alexr17 commented Apr 8, 2025 •

edited

Loading

yihua Apr 11, 2025 •

edited

Loading