wombatu-kun commented on code in PR #16847:
URL: https://github.com/apache/iceberg/pull/16847#discussion_r3432684523
##########
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:
This atomic remove cancels a prior heartbeat only when one is still tracked
for the entity - the orphaned/expired-takeover case where the holder never
released. Both new tests acquire-then-release each iteration, so release clears
the heartbeat before the next acquire and this remove returns null every time;
no existing test hits the non-null path either. The branch this PR actually
changes is therefore unexercised. Consider a focused test that leaves a
heartbeat orphaned (lock expires or its heartbeat task dies without a release),
then re-acquires and asserts the stale future is cancelled and replaced.
--
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]
