diff options
author | HÃ¥kon Hallingstad <hakon.hallingstad@gmail.com> | 2022-06-27 14:58:30 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-06-27 14:58:30 +0200 |
commit | 24c7eee36b9c251fc754e6ca51c921e97be44aeb (patch) | |
tree | 3119962f10273d1a95495f62779943fc6966340f /zkfacade | |
parent | 7f8d32a811a6f33a388a780b5b3284cf12ae8624 (diff) | |
parent | 33fdf29bf12b52f9113a9e94ae29e0bedc35686d (diff) |
Merge pull request #22476 from vespa-engine/hakonhall/remove-lockstats-debug
Remove LockStats debug
Diffstat (limited to 'zkfacade')
3 files changed, 11 insertions, 54 deletions
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/Lock.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/Lock.java index a5d2397dadb..9634d2220c0 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/curator/Lock.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/Lock.java @@ -9,9 +9,6 @@ import com.yahoo.vespa.curator.stats.ThreadLockStats; import org.apache.curator.framework.recipes.locks.InterProcessLock; import java.time.Duration; -import java.time.Instant; -import java.util.HashMap; -import java.util.Map; import java.util.concurrent.TimeUnit; /** @@ -24,26 +21,17 @@ import java.util.concurrent.TimeUnit; */ public class Lock implements Mutex { - // TODO(hakon): Remove once debugging is done - private final Object monitor = new Object(); - private long nextSequenceNumber = 0; - private final Map<Long, Long> reentriesByThreadId = new HashMap<>(); - private final Instant created = Instant.now(); - private Curator curator; - private final InterProcessLock mutex; private final String lockPath; public Lock(String lockPath, Curator curator) { this(lockPath, curator.createMutex(lockPath)); - this.curator = curator; } /** Public for testing only */ public Lock(String lockPath, InterProcessLock mutex) { this.lockPath = lockPath; this.mutex = mutex; - this.curator = null; } /** Take the lock with the given timeout. This may be called multiple times from the same thread - each matched by a close */ @@ -65,38 +53,7 @@ public class Lock implements Mutex { " to acquire lock '" + lockPath + "'"); } - invoke(+1L, (lockPath, debug) -> threadLockStats.lockAcquired(debug), lockPath); - } - - @FunctionalInterface - private interface BiConsumer2 { - void accept(String lockPath, String debug); - } - - // TODO(hakon): Remove once debugging is unnecessary - private void invoke(long reentryCountDiff, BiConsumer2 consumer, String lockPath) { - long threadId = Thread.currentThread().getId(); - final long sequenceNumber; - final Map<Long, Long> reentriesByThreadIdCopy; - synchronized (monitor) { - sequenceNumber = nextSequenceNumber++; - reentriesByThreadId.merge(threadId, reentryCountDiff, (oldValue, argumentValue) -> { - long sum = oldValue + argumentValue /* == reentryCountDiff */; - if (sum == 0) { - // Remove from map - return null; - } else { - return sum; - } - }); - reentriesByThreadIdCopy = Map.copyOf(reentriesByThreadId); - } - - String debug = "thread " + threadId + " Lock 0x" + Integer.toHexString(System.identityHashCode(this)) + - "@" + created + " Curator 0x" + Integer.toHexString(System.identityHashCode(curator)) + - " lock " + lockPath + " #" + sequenceNumber + - ", reentries by thread ID = " + reentriesByThreadIdCopy; - consumer.accept(lockPath, debug); + threadLockStats.lockAcquired(); } @Override @@ -105,7 +62,7 @@ public class Lock implements Mutex { // Update metrics now before release() to avoid double-counting time in locked state. // The lockPath must be sent down as close() may be invoked in an order not necessarily // equal to the reverse order of acquires. - invoke(-1L, threadLockStats::preRelease, lockPath); + threadLockStats.preRelease(lockPath); try { mutex.release(); threadLockStats.postRelease(lockPath); diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockStats.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockStats.java index a37034b7547..0897078b94d 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockStats.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockStats.java @@ -64,28 +64,28 @@ public class LockStats { } /** Must be invoked only after the first and non-reentry acquisition of the lock. */ - void notifyOfThreadHoldingLock(Thread currentThread, String lockPath, String debug) { + void notifyOfThreadHoldingLock(Thread currentThread, String lockPath) { Thread oldThread = lockPathsHeld.put(lockPath, currentThread); if (oldThread != null) { getLockMetrics(lockPath).incrementAcquireWithoutReleaseCount(); logger.warning("Thread " + currentThread.getName() + " reports it has the lock on " + lockPath + ", but thread " + oldThread.getName() + - " has not reported it released the lock. " + debug); + " has not reported it released the lock"); } } /** Must be invoked only before the last and non-reentry release of the lock. */ - void notifyOfThreadReleasingLock(Thread currentThread, String lockPath, String debug) { + void notifyOfThreadReleasingLock(Thread currentThread, String lockPath) { Thread oldThread = lockPathsHeld.remove(lockPath); if (oldThread == null) { getLockMetrics(lockPath).incrementNakedReleaseCount(); logger.warning("Thread " + currentThread.getName() + " is releasing the lock " + lockPath + - ", but nobody owns that lock. " + debug); + ", but nobody owns that lock"); } else if (oldThread != currentThread) { getLockMetrics(lockPath).incrementForeignReleaseCount(); logger.warning("Thread " + currentThread.getName() + " is releasing the lock " + lockPath + ", but it was owned by thread " + - oldThread.getName() + ". " + debug); + oldThread.getName()); } } diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java index 24305539be5..f0b26e61870 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java @@ -98,7 +98,7 @@ public class ThreadLockStats { } /** Mutable method (see class doc) */ - public void lockAcquired(String debug) { + public void lockAcquired() { withLastLockAttempt(lockAttempt -> { // Note on the order of lockAcquired() vs notifyOfThreadHoldingLock(): When the latter is // invoked, other threads may query e.g. isAcquired() on the lockAttempt, which would @@ -107,18 +107,18 @@ public class ThreadLockStats { lockAttempt.lockAcquired(); if (!lockAttempt.isReentry()) { - LockStats.getGlobal().notifyOfThreadHoldingLock(thread, lockAttempt.getLockPath(), debug); + LockStats.getGlobal().notifyOfThreadHoldingLock(thread, lockAttempt.getLockPath()); } }); } /** Mutable method (see class doc) */ - public void preRelease(String path, String debug) { + public void preRelease(String path) { withLastLockAttemptFor(path, lockAttempt -> { // Note on the order of these two statement: Same concerns apply here as in lockAcquired(). if (!lockAttempt.isReentry()) { - LockStats.getGlobal().notifyOfThreadReleasingLock(thread, lockAttempt.getLockPath(), debug); + LockStats.getGlobal().notifyOfThreadReleasingLock(thread, lockAttempt.getLockPath()); } lockAttempt.preRelease(); |