hudi-agent commented on code in PR #18492:
URL: https://github.com/apache/hudi/pull/18492#discussion_r3382275793
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestStorageBasedLockProvider.java:
##########
@@ -512,8 +512,50 @@ void testRenewLockSucceeds() {
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);
+ 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 successData = new StorageLockData(false, t0 +
DEFAULT_LOCK_VALIDITY_MS, ownerId);
+ StorageLockFile successLockFile = new StorageLockFile(successData, "v2");
Review Comment:
🤖 nit: could you rename `successData`/`successLockFile` to
`renewedLockData`/`renewedLockFile`? The whole point of this test is
contrasting old vs. new lease expiration, so having `oldLockFile` on one side
and `renewedLockFile` on the other makes that pairing immediately visible.
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
