[HUDI-9159] S3 implementation of StorageLock for StorageBasedLockProvider #13126

alexr17 · 2025-04-10T05:02:28Z

Change Logs

Adds the storage based lock provider implementation for s3 hudi tables.

Impact

Allows conditional writes based locking for multi writer scenarios with hudi tables in s3.

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

None

Documentation Update

None

Contributor's checklist

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

In order to test this we setup local hudi/spark bundles and ran some concurrent spark sql queries to tables with this LP enabled to ensure things functioned as expected.

…tation

…lient/transaction/lock/ConditionalWriteLockConfig.java Co-authored-by: Y Ethan Guo <[email protected]>

…lient/transaction/lock/ConditionalWriteLockProvider.java Co-authored-by: Y Ethan Guo <[email protected]>

Co-authored-by: Y Ethan Guo <[email protected]>

vinothchandar

Reviewed the S3StorageLockClient

hudi-aws/src/main/java/org/apache/hudi/aws/transaction/lock/S3StorageLockClient.java

vinothchandar · 2025-04-10T23:56:12Z

hudi-aws/src/test/java/org/apache/hudi/aws/transaction/lock/TestS3StorageBasedLockProvider.java

+ * Tests S3-based StorageBasedLockProvider using a LocalStack container
+ * to emulate S3.
+ */
+@Disabled("HUDI-9159 The tests do not work. Disabling them to unblock Azure CI")


is there plan to make this work

Should we mock s3 behavior. Looks like you are trying to functional test using localstack

Yeah we mock the s3 behavior with TestS3StorageLockClient. Localstack doesn't seem to work in Azure CI, same issue as other tests Davis disabled a while back

hudi-aws/src/test/java/org/apache/hudi/aws/transaction/lock/TestS3StorageLockClient.java

nsivabalan · 2025-04-11T17:34:40Z

can you update the PR desc as to what tests we have done so far in cluster w/ this patch?
sanity, soak testing ( to weed out connection leaks, etc)?

nsivabalan · 2025-04-11T22:15:57Z

btw, lets remove the retries within renew method. I see we do synchronized across renew method and close(). so, there are chances that close() starves, when renew() is sleeping.

so, lets remove the retries within renew method(or tryCreateOrUpdateLockFile) in let heart beat manager do the retry after 30 secs.

nsivabalan · 2025-04-11T22:17:48Z

Few more feedback that needs to be addressed:

https://github.com/apache/hudi/pull/13126/files#r2038561912
For the missing region issue, why can't we just go ahead and instantiate S3 client and then make below call to get the region and reinstantiate s3 client if need be.

GetBucketLocationResponse bucketLocationResponse = s3Client.getBucketLocation(GetBucketLocationRequest.builder().bucket(bucketName).build());
String bucketRegion = bucketLocationResponse.locationConstraint().toString();
https://github.com/apache/hudi/pull/13126/files#r2038581765
https://github.com/apache/hudi/pull/13126/files#r2038585276

hudi-aws/src/main/java/org/apache/hudi/aws/transaction/lock/S3StorageLockClient.java

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

hudi-aws/src/main/java/org/apache/hudi/aws/transaction/lock/S3StorageLockClient.java

alexr17 · 2025-04-12T20:09:08Z

@hudi-bot run azure

alexr17 · 2025-04-12T23:34:09Z

@hudi-bot run azure

hudi-aws/src/main/java/org/apache/hudi/aws/transaction/lock/S3StorageLockClient.java

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

hudi-aws/src/main/java/org/apache/hudi/aws/transaction/lock/S3StorageLockClient.java

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

hudi-bot · 2025-04-13T17:14:41Z

CI report:

99b0ac8 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

nsivabalan

thanks for patiently addressing all feedback Alex.

nsivabalan · 2025-04-13T22:48:41Z

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

@@ -83,13 +83,13 @@ private void checkRequiredProps() {
        throw new IllegalArgumentException(BASE_PATH.key() + notExistsMsg);
      }
      if (lockConfig.getLongOrDefault(VALIDITY_TIMEOUT_SECONDS) < lockConfig.getLongOrDefault(HEARTBEAT_POLL_SECONDS)
-          * 3) {
+          * 10) {


if you get any more feedback from danny or someone before we land this patch, can you also fix L 35 var name to LOCK_VALIDITY_TIMEOUT_SECS along w/ addressing them.

Actually @yihua suggested VALIDITY_TIMEOUT_SECONDS in an earlier PR it used to be LOCK_VALIDITY_TIMEOUT_MS

nsivabalan · 2025-04-13T22:49:21Z

@danny0405 : Do you wanna review this patch

danny0405 · 2025-04-14T01:37:59Z

hudi-aws/src/main/java/org/apache/hudi/aws/transaction/lock/S3StorageLockClient.java

+    } catch (S3Exception e) {
+      int status = e.statusCode();
+      // Default to unknown error
+      LockGetResult result = LockGetResult.UNKNOWN_ERROR;


rename handleUpsertS3Exception to handleS3Exception and reuse it here?

danny0405 · 2025-04-14T01:42:32Z

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

  // How long to wait before retrying lock acquisition in blocking calls.
-  private static final long DEFAULT_LOCK_ACQUISITION_BUFFER_MS = 1000;


delete the comment in line 76 too?

danny0405

+1, fine with it, just some minor comments.

…ider (apache#13126) * Adds the storage based lock provider implementation for s3 hudi tables. --------- Co-authored-by: Y Ethan Guo <[email protected]> Co-authored-by: sivabalan <[email protected]> (cherry picked from commit ed1ef93)

alexr17 and others added 19 commits April 7, 2025 20:32

Add initial changes for storage-based lock provider abstract implemen…

11d8732

…tation

fix formatting

b0d95ce

checkstyle

d403ae4

more checkstyle

2a47361

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

418335c

…lient/transaction/lock/ConditionalWriteLockConfig.java Co-authored-by: Y Ethan Guo <[email protected]>

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

6f63e44

…lient/transaction/lock/ConditionalWriteLockConfig.java Co-authored-by: Y Ethan Guo <[email protected]>

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

5fa41ad

…lient/transaction/lock/ConditionalWriteLockConfig.java Co-authored-by: Y Ethan Guo <[email protected]>

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

dc610af

…lient/transaction/lock/ConditionalWriteLockConfig.java Co-authored-by: Y Ethan Guo <[email protected]>

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

7265362

…lient/transaction/lock/ConditionalWriteLockProvider.java Co-authored-by: Y Ethan Guo <[email protected]>

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

cd3bd0b

…lient/transaction/lock/ConditionalWriteLockProvider.java Co-authored-by: Y Ethan Guo <[email protected]>

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

fd6e8a8

…lient/transaction/lock/ConditionalWriteLockProvider.java Co-authored-by: Y Ethan Guo <[email protected]>

Apply suggestions from code review

3076bb5

Co-authored-by: Y Ethan Guo <[email protected]>

respond to feedback and refactor models

d4ec22c

update config

be22bed

Merge remote-tracking branch 'upstream/master' into HUDI-9158/base-lp

7414876

rename class

015ba56

add s3 implementation for storage based lock provider

8cefedf

conditional writes rename

767cc61

Merge branch 'HUDI-9158/base-lp' into HUDI-9159/s3-impl

bdd5e22

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

alexr17 added 7 commits April 10, 2025 09:28

try tweaks

6258b15

fix test issues

18074b8

checkstyle fix

40ebfc4

add shutdown hook

88184cd

refactor configs based on feedback

c66c726

Merge branch 'HUDI-9158/base-lp' into HUDI-9159/s3-impl

6ed45f1

fix import

422232e

vinothchandar reviewed Apr 10, 2025

View reviewed changes

alexr17 added 2 commits April 10, 2025 17:55

cleanup lp initialization based on feedback

b917d9f

checkstyle

b133ae0

yihua and others added 4 commits April 10, 2025 23:41

Address nits

8f21de8

Merge branch 'HUDI-9158/base-lp' into HUDI-9159/s3-impl

9b56b56

Merge remote-tracking branch 'upstream/master' into HUDI-9159/s3-impl

57d5d17

consume latest upstream changes and address some feedback

2801116

Addressing feedback from siva

d218f8f

nsivabalan force-pushed the HUDI-9159/s3-impl branch from 13b4575 to d218f8f Compare April 11, 2025 21:28

nsivabalan and others added 3 commits April 11, 2025 16:02

minor nits

1e8ad76

respond to all feedback with very large refactor

d6483b9

checkstyle

0c81849

nsivabalan reviewed Apr 12, 2025

View reviewed changes

update s3 client initialization based on feedback

f12ce76

nsivabalan reviewed Apr 13, 2025

View reviewed changes

respond to nits

edc44bd

nsivabalan reviewed Apr 13, 2025

View reviewed changes

respond to few more nits

99b0ac8

nsivabalan approved these changes Apr 13, 2025

View reviewed changes

nsivabalan reviewed Apr 13, 2025

View reviewed changes

danny0405 reviewed Apr 14, 2025

View reviewed changes

danny0405 approved these changes Apr 14, 2025

View reviewed changes

danny0405 merged commit ed1ef93 into apache:master Apr 14, 2025
60 checks passed

voonhous added the release-1.0.2 label Apr 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-9159] S3 implementation of StorageLock for StorageBasedLockProvider #13126

[HUDI-9159] S3 implementation of StorageLock for StorageBasedLockProvider #13126

alexr17 commented Apr 10, 2025 •

edited

Loading

vinothchandar left a comment

vinothchandar Apr 10, 2025

vinothchandar Apr 10, 2025

alexr17 Apr 11, 2025

nsivabalan commented Apr 11, 2025

nsivabalan commented Apr 11, 2025

nsivabalan commented Apr 11, 2025 •

edited

Loading

alexr17 commented Apr 12, 2025

alexr17 commented Apr 12, 2025

hudi-bot commented Apr 13, 2025

nsivabalan left a comment

nsivabalan Apr 13, 2025 •

edited

Loading

alexr17 Apr 14, 2025

nsivabalan commented Apr 13, 2025

danny0405 Apr 14, 2025

danny0405 Apr 14, 2025

danny0405 left a comment

		// How long to wait before retrying lock acquisition in blocking calls.
		private static final long DEFAULT_LOCK_ACQUISITION_BUFFER_MS = 1000;

[HUDI-9159] S3 implementation of StorageLock for StorageBasedLockProvider #13126

[HUDI-9159] S3 implementation of StorageLock for StorageBasedLockProvider #13126

Conversation

alexr17 commented Apr 10, 2025 • edited Loading

Change Logs

Impact

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

Documentation Update

Contributor's checklist

vinothchandar left a comment

Choose a reason for hiding this comment

vinothchandar Apr 10, 2025

Choose a reason for hiding this comment

vinothchandar Apr 10, 2025

Choose a reason for hiding this comment

alexr17 Apr 11, 2025

Choose a reason for hiding this comment

nsivabalan commented Apr 11, 2025

nsivabalan commented Apr 11, 2025

nsivabalan commented Apr 11, 2025 • edited Loading

alexr17 commented Apr 12, 2025

alexr17 commented Apr 12, 2025

hudi-bot commented Apr 13, 2025

CI report:

nsivabalan left a comment

Choose a reason for hiding this comment

nsivabalan Apr 13, 2025 • edited Loading

Choose a reason for hiding this comment

alexr17 Apr 14, 2025

Choose a reason for hiding this comment

nsivabalan commented Apr 13, 2025

danny0405 Apr 14, 2025

Choose a reason for hiding this comment

danny0405 Apr 14, 2025

Choose a reason for hiding this comment

danny0405 left a comment

Choose a reason for hiding this comment

alexr17 commented Apr 10, 2025 •

edited

Loading

nsivabalan commented Apr 11, 2025 •

edited

Loading

nsivabalan Apr 13, 2025 •

edited

Loading