This is an automated email from the ASF dual-hosted git repository.
voonhous pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hudi.git
The following commit(s) were added to refs/heads/master by this push:
new 42e938dbd287 fix: Fix lock expiration metric (#18492)
42e938dbd287 is described below
commit 42e938dbd2874086c07234faffe278b3be3354c7
Author: Lin Liu <[email protected]>
AuthorDate: Sun Jul 26 03:50:50 2026 -0700
fix: Fix lock expiration metric (#18492)
* Fix metric
* Address review: read the clock once when reporting the renewed lock
deadline
The SUCCESS branch of renewLock called getCurrentEpochMs() three times, so
the
value fed to updateLockExpirationDeadlineMetric and the two values in the
log
line could each be computed against a different clock read. Capture the
clock
once and derive both the metric and the log arguments from it.
---------
Co-authored-by: voon <[email protected]>
---
.../transaction/lock/StorageBasedLockProvider.java | 20 ++++++----
.../lock/TestStorageBasedLockProvider.java | 45 +++++++++++++++++++++-
2 files changed, 56 insertions(+), 9 deletions(-)
diff --git
a/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java
b/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java
index d170eb846572..974ea01aee7a 100644
---
a/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java
+++
b/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java
@@ -630,16 +630,22 @@ public class StorageBasedLockProvider implements
LockProvider<StorageLockFile> {
hoodieLockMetrics.ifPresent(HoodieLockMetrics::updateLockThrottledMetric);
// Let heartbeat retry later.
return true;
- case SUCCESS:
- // Only positive outcome
- this.setLock(currentLock.getRight().get());
- hoodieLockMetrics.ifPresent(metrics ->
metrics.updateLockExpirationDeadlineMetric(
- (int) (oldExpirationMs - getCurrentEpochMs())));
- logger.info("Owner {}: Lock renewal successful. The renewal
completes {} ms before expiration for lock {}.",
- ownerId, oldExpirationMs - getCurrentEpochMs(), lockFilePath);
+ case SUCCESS: {
+ // Only positive outcome. Source the deadline metric and log from
the renewed lock file
+ // returned by the storage client (same as the acquisition path),
not the locally
+ // computed expiration, so both callers agree on where the deadline
comes from.
+ StorageLockFile renewedLock = currentLock.getRight().get();
+ this.setLock(renewedLock);
+ // Read the clock once so the metric and the log line below report
the same deadline.
+ long renewalCompletionMs = getCurrentEpochMs();
+ long remainingLeaseMs = renewedLock.getValidUntilMs() -
renewalCompletionMs;
+ hoodieLockMetrics.ifPresent(metrics ->
metrics.updateLockExpirationDeadlineMetric((int) remainingLeaseMs));
+ logger.info("Owner {}: Lock renewal successful. The renewal
completes {} ms before old expiration. The lock will expire in {} ms for lock
{}.",
+ ownerId, oldExpirationMs - renewalCompletionMs,
remainingLeaseMs, lockFilePath);
recordAuditOperation(AuditOperationState.RENEW,
acquisitionTimestamp);
// Let heartbeat continue to renew lock lease again later.
return true;
+ }
default:
throw new HoodieLockException("Unexpected lock update result: " +
currentLock.getLeft());
}
diff --git
a/hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestStorageBasedLockProvider.java
b/hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestStorageBasedLockProvider.java
index 9aad86282394..85c21c129d0f 100644
---
a/hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestStorageBasedLockProvider.java
+++
b/hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestStorageBasedLockProvider.java
@@ -619,8 +619,49 @@ class TestStorageBasedLockProvider {
assertTrue(lockProvider.renewLock());
verify(mockLogger).info(
- eq("Owner {}: Lock renewal successful. The renewal completes {} ms
before expiration for lock {}."),
- eq(this.ownerId), anyLong(),
eq("gs://bucket/lake/db/tbl-default/.hoodie/.locks/table_lock.json"));
+ eq("Owner {}: Lock renewal successful. The renewal completes {} ms
before old expiration. The lock will expire in {} ms for lock {}."),
+ eq(this.ownerId), anyLong(), anyLong(),
eq("gs://bucket/lake/db/tbl-default/.hoodie/.locks/table_lock.json"));
+ }
+
+ @Test
+ void testRenewLockMetricUsesNewLeaseExpiration() {
+ // Regression test for https://github.com/apache/hudi/issues/18493: after
a successful
+ // renewal the lock.expiration.deadline metric must reflect the remaining
time on the newly
+ // renewed lease, not on the previous (about-to-expire) lease.
+ TypedProperties props = new TypedProperties();
+ props.put(StorageBasedLockConfig.VALIDITY_TIMEOUT_SECONDS.key(), "10");
+ props.put(StorageBasedLockConfig.RENEW_INTERVAL_SECS.key(), "1");
+ props.put(BASE_PATH.key(), "gs://bucket/lake/db/tbl-default");
+
+ HoodieLockMetrics mockMetrics = mock(HoodieLockMetrics.class);
+ try (StorageBasedLockProvider providerWithMetrics = spy(new
StorageBasedLockProvider(
+ ownerId,
+ props,
+ (a, b, c) -> mockHeartbeatManager,
+ (a, b, c) -> mockLockService,
+ mockLogger,
+ mockMetrics))) {
+
+ long t0 = 100_000L;
+ when(providerWithMetrics.getCurrentEpochMs()).thenReturn(t0);
+
+ // The currently held lease is about to expire (only 100 ms left) --
this is what makes the
+ // old vs new distinction observable: the old lease would have reported
~100 ms.
+ StorageLockData oldData = new StorageLockData(false, t0 + 100, ownerId);
+ StorageLockFile oldLockFile = new StorageLockFile(oldData, "v1");
+ doReturn(oldLockFile).when(providerWithMetrics).getLock();
+
+ StorageLockData renewedLockData = new StorageLockData(false, t0 +
DEFAULT_LOCK_VALIDITY_MS, ownerId);
+ StorageLockFile renewedLockFile = new StorageLockFile(renewedLockData,
"v2");
+ when(mockLockService.tryUpsertLockFile(any(),
eq(Option.of(oldLockFile))))
+ .thenReturn(Pair.of(LockUpsertResult.SUCCESS,
Option.of(renewedLockFile)));
+
+ assertTrue(providerWithMetrics.renewLock());
+
+ // New lease = t0 + 10000ms validity, evaluated at t0 -> 10000 ms
remaining (the fix),
+ // not the ~100 ms remaining on the old lease (the bug).
+
verify(mockMetrics).updateLockExpirationDeadlineMetric(DEFAULT_LOCK_VALIDITY_MS);
+ }
}
@Test
