[HUDI-9158] Add storage-based lock provider abstract implementation by alexr17 · Pull Request #13103 · apache/hudi

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

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

+ 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.

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

@hudi-bot run azure

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

@hudi-bot run azure

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

 * @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.

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

 @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.

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

+ // 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.

danny0405

+1, LGTM.

yihua

Blocked on my final review

nsivabalan

minor nits. LGTM.

…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>

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

+ 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.

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

+ // 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-actionsBot 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

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

yihua reviewed Apr 8, 2025
View reviewed changes

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

yihua reviewed Apr 8, 2025
View reviewed changes

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

yihua reviewed Apr 8, 2025
View reviewed changes

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

yihua reviewed Apr 8, 2025
View reviewed changes

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

yihua reviewed Apr 8, 2025
View reviewed changes

Comment threadhudi-io/src/main/java/org/apache/hudi/storage/StorageSchemes.java Outdated
Comment threadhudi-hadoop-common/src/test/java/org/apache/hudi/common/fs/TestStorageSchemes.java