-
Notifications
You must be signed in to change notification settings - Fork 15
Add new getLockState() debugging endpoint #5302
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
type: improvement | ||
improvement: | ||
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. | ||
links: | ||
- https://github.com/palantir/atlasdb/pull/5302 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
/* | ||
* (c) Copyright 2021 Palantir Technologies Inc. All rights reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package com.palantir.lock; | ||
|
||
import com.fasterxml.jackson.databind.annotation.JsonDeserialize; | ||
import com.fasterxml.jackson.databind.annotation.JsonSerialize; | ||
import java.util.List; | ||
import java.util.Optional; | ||
import org.immutables.value.Value; | ||
|
||
@Value.Immutable | ||
@JsonSerialize(as = ImmutableLockState.class) | ||
@JsonDeserialize(as = ImmutableLockState.class) | ||
public interface LockState { | ||
boolean isWriteLocked(); | ||
|
||
boolean isFrozen(); | ||
|
||
List<LockClient> exactCurrentLockHolders(); | ||
|
||
List<LockHolder> holders(); | ||
|
||
List<LockRequester> requesters(); | ||
|
||
@Value.Immutable | ||
@JsonSerialize(as = ImmutableLockHolder.class) | ||
@JsonDeserialize(as = ImmutableLockHolder.class) | ||
interface LockHolder { | ||
LockClient client(); | ||
|
||
long creationDateMs(); | ||
|
||
long expirationDateMs(); | ||
|
||
int numOtherLocksHeld(); | ||
|
||
Optional<Long> versionId(); | ||
|
||
String requestingThread(); | ||
} | ||
|
||
@Value.Immutable | ||
@JsonSerialize(as = ImmutableLockRequester.class) | ||
@JsonDeserialize(as = ImmutableLockRequester.class) | ||
interface LockRequester { | ||
LockClient client(); | ||
|
||
LockGroupBehavior lockGroupBehavior(); | ||
|
||
BlockingMode blockingMode(); | ||
|
||
Optional<TimeDuration> blockingDuration(); | ||
|
||
Optional<Long> versionId(); | ||
|
||
String requestingThread(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,12 +19,14 @@ | |
import com.palantir.lock.HeldLocksGrant; | ||
import com.palantir.lock.HeldLocksToken; | ||
import com.palantir.lock.LockClient; | ||
import com.palantir.lock.LockDescriptor; | ||
import com.palantir.lock.LockRefreshToken; | ||
import com.palantir.lock.LockRequest; | ||
import com.palantir.lock.LockResponse; | ||
import com.palantir.lock.LockRpcClient; | ||
import com.palantir.lock.LockServerOptions; | ||
import com.palantir.lock.LockService; | ||
import com.palantir.lock.LockState; | ||
import com.palantir.lock.NamespaceAgnosticLockRpcClient; | ||
import com.palantir.lock.SimpleHeldLocksToken; | ||
import java.math.BigInteger; | ||
|
@@ -157,4 +159,9 @@ public long currentTimeMillis() { | |
public void logCurrentState() { | ||
namespaceAgnosticLockRpcClient.logCurrentState(); | ||
} | ||
|
||
@Override | ||
public LockState getLockState(LockDescriptor lock) { | ||
return null; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this right? If so, can we have a comment to explain? |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,10 @@ public LockServerLock(LockDescriptor descriptor, LockClientIndices clients) { | |
this.sync = new LockServerSync(clients); | ||
} | ||
|
||
/* package */ LockServerSync getSync() { | ||
return sync; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we remove the package comment for uniformity. Also can we move the package-private method below public methods? |
||
@Override | ||
public LockDescriptor getDescriptor() { | ||
return descriptor; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -260,7 +260,7 @@ private synchronized void decrementReadCount(int clientIndex) { | |
} | ||
} | ||
|
||
private synchronized Iterable<Integer> getReadClients() { | ||
/* package */ synchronized Iterable<Integer> getReadClients() { | ||
if (readLockHolders == null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we don't use the "package" comment anywhere else in this class |
||
return ImmutableList.of(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -50,6 +50,9 @@ | |||||||||||||||||||||||||||||||
import com.palantir.lock.HeldLocksGrant; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.HeldLocksToken; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.HeldLocksTokens; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.ImmutableLockHolder; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.ImmutableLockRequester; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.ImmutableLockState; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.LockClient; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.LockCollection; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.LockCollections; | ||||||||||||||||||||||||||||||||
|
@@ -62,6 +65,7 @@ | |||||||||||||||||||||||||||||||
import com.palantir.lock.LockServerConfigs; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.LockServerOptions; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.LockService; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.LockState; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.RemoteLockService; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.SimpleHeldLocksToken; | ||||||||||||||||||||||||||||||||
import com.palantir.lock.SimpleTimeDuration; | ||||||||||||||||||||||||||||||||
|
@@ -85,6 +89,7 @@ | |||||||||||||||||||||||||||||||
import java.util.LinkedList; | ||||||||||||||||||||||||||||||||
import java.util.List; | ||||||||||||||||||||||||||||||||
import java.util.Map; | ||||||||||||||||||||||||||||||||
import java.util.Optional; | ||||||||||||||||||||||||||||||||
import java.util.Set; | ||||||||||||||||||||||||||||||||
import java.util.concurrent.BlockingQueue; | ||||||||||||||||||||||||||||||||
import java.util.concurrent.ConcurrentHashMap; | ||||||||||||||||||||||||||||||||
|
@@ -1128,6 +1133,73 @@ public LockServerOptions getLockServerOptions() { | |||||||||||||||||||||||||||||||
return options; | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
@Override | ||||||||||||||||||||||||||||||||
public LockState getLockState(LockDescriptor descriptor) { | ||||||||||||||||||||||||||||||||
LockServerLock readWriteLock = (LockServerLock) descriptorToLockMap.getIfPresent(descriptor); | ||||||||||||||||||||||||||||||||
if (readWriteLock == null) { | ||||||||||||||||||||||||||||||||
return ImmutableLockState.builder() | ||||||||||||||||||||||||||||||||
.isWriteLocked(false) | ||||||||||||||||||||||||||||||||
.isFrozen(false) | ||||||||||||||||||||||||||||||||
.build(); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Declare a constant? |
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
LockServerSync sync = readWriteLock.getSync(); | ||||||||||||||||||||||||||||||||
List<LockClient> readHolders; | ||||||||||||||||||||||||||||||||
LockClient writeHolders; | ||||||||||||||||||||||||||||||||
boolean isFrozen; | ||||||||||||||||||||||||||||||||
boolean writeMode; | ||||||||||||||||||||||||||||||||
synchronized (sync) { | ||||||||||||||||||||||||||||||||
readHolders = ImmutableList.copyOf(Iterables.transform(sync.getReadClients(), clientIndices::fromIndex)); | ||||||||||||||||||||||||||||||||
writeHolders = sync.getLockHolder(); | ||||||||||||||||||||||||||||||||
isFrozen = sync.isFrozen(); | ||||||||||||||||||||||||||||||||
writeMode = readHolders.isEmpty(); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
List<LockClient> lockHolders; | ||||||||||||||||||||||||||||||||
if (readHolders.isEmpty()) { | ||||||||||||||||||||||||||||||||
if (writeHolders == null) { | ||||||||||||||||||||||||||||||||
lockHolders = ImmutableList.of(); | ||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||
lockHolders = ImmutableList.of(writeHolders); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||
lockHolders = readHolders; | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extract out method, maybe something like -
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like a lot of fanciness for not a lot of benefit, but either way's fine by me! |
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
List<HeldLocksToken> heldLocks = new ArrayList<>(); | ||||||||||||||||||||||||||||||||
heldLocksTokenMap.keySet().stream() | ||||||||||||||||||||||||||||||||
.filter(token -> token.getLockDescriptors().contains(descriptor)) | ||||||||||||||||||||||||||||||||
.forEach(heldLocks::add); | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
Map<LockRequest, LockClient> requests = new HashMap<>(); | ||||||||||||||||||||||||||||||||
outstandingLockRequestMultimap.forEach((client, request) -> { | ||||||||||||||||||||||||||||||||
if (request.getLockDescriptors().contains(descriptor)) { | ||||||||||||||||||||||||||||||||
requests.put(request, client); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
ImmutableLockState.Builder lockState = ImmutableLockState.builder() | ||||||||||||||||||||||||||||||||
.isWriteLocked(writeMode) | ||||||||||||||||||||||||||||||||
.exactCurrentLockHolders(lockHolders) | ||||||||||||||||||||||||||||||||
.isFrozen(isFrozen); | ||||||||||||||||||||||||||||||||
heldLocks.forEach(lock -> lockState.addHolders(ImmutableLockHolder.builder() | ||||||||||||||||||||||||||||||||
.client(lock.getClient()) | ||||||||||||||||||||||||||||||||
.creationDateMs(lock.getCreationDateMs()) | ||||||||||||||||||||||||||||||||
.expirationDateMs(lock.getExpirationDateMs()) | ||||||||||||||||||||||||||||||||
.numOtherLocksHeld(lock.getLocks().size() - 1) | ||||||||||||||||||||||||||||||||
.versionId(Optional.ofNullable(lock.getVersionId())) | ||||||||||||||||||||||||||||||||
.requestingThread(lock.getRequestingThread()) | ||||||||||||||||||||||||||||||||
.build())); | ||||||||||||||||||||||||||||||||
requests.forEach((request, client) -> lockState.addRequesters(ImmutableLockRequester.builder() | ||||||||||||||||||||||||||||||||
.client(client) | ||||||||||||||||||||||||||||||||
.lockGroupBehavior(request.getLockGroupBehavior()) | ||||||||||||||||||||||||||||||||
.blockingMode(request.getBlockingMode()) | ||||||||||||||||||||||||||||||||
.blockingDuration(Optional.ofNullable(request.getBlockingDuration())) | ||||||||||||||||||||||||||||||||
.versionId(Optional.ofNullable(request.getVersionId())) | ||||||||||||||||||||||||||||||||
.requestingThread(request.getCreatingThreadName()) | ||||||||||||||||||||||||||||||||
.build())); | ||||||||||||||||||||||||||||||||
return lockState.build(); | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can extract util methods for above; these static methods could live in LockState There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||
* Prints the current state of the lock server to the logs. Useful for | ||||||||||||||||||||||||||||||||
* debugging. | ||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surely
namespaceAgnosticLockRpcClient.getLockState(lock);
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops!