voonhous commented on code in PR #19370:
URL: https://github.com/apache/hudi/pull/19370#discussion_r3644482056
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/transaction/lock/HiveMetastoreBasedLockProvider.java:
##########
@@ -225,12 +234,33 @@ private void acquireLockInternal(long time, TimeUnit
unit, LockComponent lockCom
* takes a long time. Must be called only after {@link #lock} has been set.
*/
private void scheduleHeartbeat() {
- Heartbeat heartbeat = new Heartbeat(hiveClient, lock.getLockid());
+ Heartbeat heartbeat = new Heartbeat(hiveClient, lock.getLockid(),
this::onLockLost);
Review Comment:
The lock-id guard in 25abde04 covers the realistic path here: a stale
WAITING tick that fires after the `finally` released the lock now fails the
`lockId` check and gets ignored, and
`releasingALockNormallyIsNotReportedAsLost` pins that behavior down. What
remains is only the pre-release window -- a tick firing in the microseconds
before the `finally` while `lock` still matches, which would also need HMS to
have forgotten a lock it granted moments earlier -- not reachable in practice,
and the next acquire resets the flag anyway.
So no longer a correctness ask. Gating on `lock.getState() ==
LockState.ACQUIRED` would still avoid scheduling a heartbeat the `finally`
immediately cancels, but that is a nit. Fine to resolve as-is.
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/transaction/lock/HiveMetastoreBasedLockProvider.java:
##########
@@ -225,12 +234,33 @@ private void acquireLockInternal(long time, TimeUnit
unit, LockComponent lockCom
* takes a long time. Must be called only after {@link #lock} has been set.
*/
private void scheduleHeartbeat() {
- Heartbeat heartbeat = new Heartbeat(hiveClient, lock.getLockid());
+ Heartbeat heartbeat = new Heartbeat(hiveClient, lock.getLockid(),
this::onLockLost);
Review Comment:
This also gets scheduled for a lock that came back `WAITING` -- the
`finally` at line 224 is about to release it, but the heartbeat is created
first. Before this PR that was harmless; now, if such a tick ever fires before
the cancel, it gets `NoSuchLockException` for the just-released lock and
latches `lockLostRemotely` for a lock we never held, so a later unrelated
`unlock()` throws. Narrow window (first tick is interval/2 away), but there is
no reason to heartbeat a lock we are about to give back.
Call `scheduleHeartbeat()` only when `lock.getState() ==
LockState.ACQUIRED`, and ideally add a WAITING-lock test whose heartbeat throws
`NoSuchLockException`, asserting the later `unlock()` stays silent.
--
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]
