Add new getLockState() debugging endpoint #5302

dxiao · 2021-03-05T23:40:33Z

Allows for easier, more targetted debugging than would be
possible with previous logCurrentState() endpoint

Allows for easier, more targetted debugging than would be possible with previous logCurrentState() endpoint

changelog-app · 2021-03-05T23:40:37Z

Generate changelog in `changelog/@unreleased`

Type

Description

Add new getLockState(LockDescriptor) debugging endpoint to the RemoteLockService, which allows for easier, more targeted debugging than would be possible with previous logCurrentState() endpoint. Provide a LockDescriptor, and get the current set of clients holding and waiting for the lock.

Check the box to generate changelog(s)

Generate changelog entry

sudiksha27

Given this is a best-effort debugging endpoint, broadly looks okay.
I have a few questions + refactoring suggestions.

@jeremyk-91 would appreciate +2 on this one.

sudiksha27 · 2021-03-09T20:17:23Z

lock-impl/src/main/java/com/palantir/lock/impl/LockServiceImpl.java

+        Map<LockRequest, LockClient> requests = new HashMap<>();
+        outstandingLockRequestMultimap.forEach((client, request) -> {
+            if (request.getLockDescriptors().contains(descriptor)) {
+                requests.put(request, client);
+            }
+        });


Suggested change

Map<LockRequest, LockClient> requests = new HashMap<>();

outstandingLockRequestMultimap.forEach((client, request) -> {

if (request.getLockDescriptors().contains(descriptor)) {

requests.put(request, client);

}

});

Map<LockRequest, LockClient> requests = KeyedStream.stream(outstandingLockRequestMultimap)

.filterEntries((client, request) -> request.getLockDescriptors().contains(descriptor))

.mapEntries((client, request) -> Maps.immutableEntry(request, client))

.collectToMap();

sudiksha27 · 2021-03-09T20:18:06Z

lock-impl/src/main/java/com/palantir/lock/impl/LockServiceImpl.java

+        List<HeldLocksToken> heldLocks = new ArrayList<>();
+        heldLocksTokenMap.keySet().stream()
+                .filter(token -> token.getLockDescriptors().contains(descriptor))
+                .forEach(heldLocks::add);


Suggested change

List<HeldLocksToken> heldLocks = new ArrayList<>();

heldLocksTokenMap.keySet().stream()

.filter(token -> token.getLockDescriptors().contains(descriptor))

.forEach(heldLocks::add);

List<HeldLocksToken> heldLocks = heldLocksTokenMap.keySet().stream()

.filter(token -> token.getLockDescriptors().contains(descriptor))

.collect(Collectors.toList());

sudiksha27 · 2021-03-09T20:27:31Z

lock-impl/src/main/java/com/palantir/lock/impl/LockServiceImpl.java

+                .versionId(Optional.ofNullable(request.getVersionId()))
+                .requestingThread(request.getCreatingThreadName())
+                .build()));
+        return lockState.build();


Maybe we can extract util methods for above; these static methods could live in LockState

I think factories for creating a LockHolder/LockRequester from the request and/or lock object would be nice, though requests.forEach(...) in the body here seems fine to me

sudiksha27 · 2021-03-09T20:28:35Z

lock-impl/src/main/java/com/palantir/lock/impl/LockServiceImpl.java

+                    .isWriteLocked(false)
+                    .isFrozen(false)
+                    .build();
+        }


Declare a constant?

sudiksha27 · 2021-03-09T20:43:00Z

lock-impl/src/main/java/com/palantir/lock/impl/LockServiceImpl.java

+        List<LockClient> lockHolders;
+        if (readHolders.isEmpty()) {
+            if (writeHolders == null) {
+                lockHolders = ImmutableList.of();
+            } else {
+                lockHolders = ImmutableList.of(writeHolders);
+            }
+        } else {
+            lockHolders = readHolders;
+        }


Extract out method, maybe something like -

Suggested change

List<LockClient> lockHolders;

if (readHolders.isEmpty()) {

if (writeHolders == null) {

lockHolders = ImmutableList.of();

} else {

lockHolders = ImmutableList.of(writeHolders);

}

} else {

lockHolders = readHolders;

}

private static List<LockClient> getLockClients(List<LockClient> readHolders, LockClient writeHolders) {

return readHolders.isEmpty()

? Optional.ofNullable(writeHolders).map(ImmutableList::of).orElseGet(ImmutableList::of)

: readHolders;

}

Seems like a lot of fanciness for not a lot of benefit, but either way's fine by me!

sudiksha27 · 2021-03-09T20:56:32Z

lock-impl/src/test/java/com/palantir/lock/LockServiceTest.java

+         * -The logCurrentState tryLock() call times out after LOG_STATE_DEBUG_LOCK_WAIT_TIME_IN_MILLIS
+         *   and the call moves on to logCurrentStateInconsistent()
+         */
+        barrier.await();


I think the comment needs to be modified?

Hmm. On reread, it's not needed at all. removing

sudiksha27 · 2021-03-10T09:52:07Z

lock-impl/src/main/java/com/palantir/lock/impl/LockServerSync.java

@@ -260,7 +260,7 @@ private synchronized void decrementReadCount(int clientIndex) {
        }
    }

-    private synchronized Iterable<Integer> getReadClients() {
+    /* package */ synchronized Iterable<Integer> getReadClients() {
        if (readLockHolders == null) {


nit: we don't use the "package" comment anywhere else in this class

sudiksha27 · 2021-03-10T09:53:55Z

lock-impl/src/main/java/com/palantir/lock/impl/LockServerLock.java

+    /* package */ LockServerSync getSync() {
+        return sync;
+    }
+


nit: can we remove the package comment for uniformity. Also can we move the package-private method below public methods?

sudiksha27 · 2021-03-10T09:54:53Z

lock-api/src/main/java/com/palantir/lock/client/RemoteLockServiceAdapter.java

+    @Override
+    public LockState getLockState(LockDescriptor lock) {
+        return null;
+    }


is this right? If so, can we have a comment to explain?

jeremyk-91

Broadly makes sense, though I'm pretty sure RLSA needs to be different. I think @sudiksha27 had a number of good suggestions.

jeremyk-91 · 2021-03-08T12:44:02Z

lock-api/src/main/java/com/palantir/lock/client/RemoteLockServiceAdapter.java

+
+    @Override
+    public LockState getLockState(LockDescriptor lock) {
+        return null;


surely namespaceAgnosticLockRpcClient.getLockState(lock);?

jeremyk-91 · 2021-03-10T11:31:45Z

lock-impl/src/main/java/com/palantir/lock/impl/LockServiceImpl.java

+                .versionId(Optional.ofNullable(request.getVersionId()))
+                .requestingThread(request.getCreatingThreadName())
+                .build()));
+        return lockState.build();


I think factories for creating a LockHolder/LockRequester from the request and/or lock object would be nice, though requests.forEach(...) in the body here seems fine to me

dxiao · 2021-03-12T04:02:19Z

Updated! Let me know if there's anything else to do. Otherwise, would live to get this in time for the weekly release :)

jeremyk-91

👍 let's merge this!

svc-autorelease · 2021-03-12T17:47:54Z

Released 0.307.2

Add new getLockState() debugging endpoint

7504270

Allows for easier, more targetted debugging than would be possible with previous logCurrentState() endpoint

dxiao requested a review from jeremyk-91 March 5, 2021 23:40

David Xiao added 3 commits March 5, 2021 15:43

fixup compile breaks

d77aa82

Add generated changelog entries

bffa9fe

spotless

27cebe1

sudiksha27 reviewed Mar 10, 2021

View reviewed changes

jeremyk-91 reviewed Mar 10, 2021

View reviewed changes

Address PR comments

9a33b60

spotless

8d0b85a

jeremyk-91 approved these changes Mar 12, 2021

View reviewed changes

jeremyk-91 added autorelease merge when ready labels Mar 12, 2021

bulldozer-bot bot merged commit c5fd8d1 into develop Mar 12, 2021

bulldozer-bot bot deleted the features/get-lock-state branch March 12, 2021 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new getLockState() debugging endpoint #5302

Add new getLockState() debugging endpoint #5302

dxiao commented Mar 5, 2021

changelog-app bot commented Mar 5, 2021 •

edited by dxiao

Loading

sudiksha27 left a comment

sudiksha27 Mar 9, 2021

sudiksha27 Mar 9, 2021

sudiksha27 Mar 9, 2021

jeremyk-91 Mar 10, 2021

sudiksha27 Mar 9, 2021

sudiksha27 Mar 9, 2021 •

edited

Loading

dxiao Mar 12, 2021

sudiksha27 Mar 9, 2021

dxiao Mar 12, 2021

sudiksha27 Mar 10, 2021

sudiksha27 Mar 10, 2021

sudiksha27 Mar 10, 2021

jeremyk-91 left a comment •

edited

Loading

jeremyk-91 Mar 8, 2021

dxiao Mar 12, 2021

jeremyk-91 Mar 10, 2021

dxiao commented Mar 12, 2021

jeremyk-91 left a comment

svc-autorelease commented Mar 12, 2021

Add new getLockState() debugging endpoint #5302

Add new getLockState() debugging endpoint #5302

Conversation

dxiao commented Mar 5, 2021

changelog-app bot commented Mar 5, 2021 • edited by dxiao Loading

Generate changelog in changelog/@unreleased

sudiksha27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudiksha27 Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk-91 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dxiao commented Mar 12, 2021

jeremyk-91 left a comment

Choose a reason for hiding this comment

svc-autorelease commented Mar 12, 2021

changelog-app bot commented Mar 5, 2021 •

edited by dxiao

Loading

Generate changelog in `changelog/@unreleased`

sudiksha27 Mar 9, 2021 •

edited

Loading

jeremyk-91 left a comment •

edited

Loading