LuciferYang commented on code in PR #16847:
URL: https://github.com/apache/iceberg/pull/16847#discussion_r3432725123
##########
core/src/main/java/org/apache/iceberg/util/LockManagers.java:
##########
@@ -206,9 +211,10 @@ void acquireOnce(String entityId, String ownerId) {
}
if (succeed) {
- // cleanup old heartbeat
- if (HEARTBEATS.containsKey(entityId)) {
- HEARTBEATS.remove(entityId).cancel(false);
+ // cleanup old heartbeat, using an atomic remove to avoid a
check-then-act race
+ ScheduledFuture<?> previousHeartbeat = HEARTBEATS.remove(entityId);
Review Comment:
Good catch, you're right. `release()` always removes the heartbeat first, so
both new tests only ever hit the null path and the branch this PR changes
stayed uncovered.
I added `testAcquireCancelsOrphanedHeartbeatOnTakeover`: it acquires without
releasing, lets the lock expire (long heartbeat interval so it isn't renewed),
then re-acquires with a new owner. That reaches the non-null branch, and the
test asserts the stale future is cancelled and replaced by a fresh one. Needed
a small `@VisibleForTesting heartbeat(entityId)` accessor to check the future
identity. Thanks for the review!
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
