Conversation
szetszwo
reviewed
Apr 4, 2025
| */ | ||
| public final class LockInfo implements Comparable<LockInfo> { | ||
| private final String key; | ||
| private final boolean isWriteLock; |
There was a problem hiding this comment.
Use an enum is better than a boolean; see "Never Use Booleans for Something That Has Two States Now, but Might Have More Later".
Comment on lines
+73
to
+107
| public AutoCloseable lock(OmLockInfo lockInfo) throws InterruptedException, TimeoutException { | ||
| // Common case is 1 volume, 1 bucket, and at most 2 key locks. | ||
| List<Lock> locks = new ArrayList<>(4); | ||
|
|
||
| Optional<LockInfo> optionalVolumeLock = lockInfo.getVolumeLock(); | ||
| Optional<LockInfo> optionalBucketLock = lockInfo.getBucketLock(); | ||
| Optional<Set<LockInfo>> optionalKeyLocks = lockInfo.getKeyLocks(); | ||
|
|
||
| if (optionalVolumeLock.isPresent()) { | ||
| LockInfo volumeLockInfo = optionalVolumeLock.get(); | ||
| if (volumeLockInfo.isWriteLock()) { | ||
| locks.add(volumeLocks.get(volumeLockInfo.getKey()).writeLock()); | ||
| } else { | ||
| locks.add(volumeLocks.get(volumeLockInfo.getKey()).readLock()); | ||
| } | ||
| } | ||
|
|
||
| if (optionalBucketLock.isPresent()) { | ||
| LockInfo bucketLockInfo = optionalBucketLock.get(); | ||
| if (bucketLockInfo.isWriteLock()) { | ||
| locks.add(bucketLocks.get(bucketLockInfo.getKey()).writeLock()); | ||
| } else { | ||
| locks.add(bucketLocks.get(bucketLockInfo.getKey()).readLock()); | ||
| } | ||
| } | ||
|
|
||
| if (optionalKeyLocks.isPresent()) { | ||
| for (Lock keyLock: keyLocks.bulkGet(optionalKeyLocks.get())) { | ||
| locks.add(keyLock); | ||
| } | ||
| } | ||
|
|
||
| acquireLocks(locks); | ||
| return () -> releaseLocks(locks); | ||
| } |
There was a problem hiding this comment.
The lock overhead probably is quite high in this method.
|
|
||
| @Override | ||
| public int compareTo(@NotNull LockInfo other) { | ||
| return Integer.compare(hashCode(), other.hashCode()); |
There was a problem hiding this comment.
Sort by hashCode won't work with guava Striped. For example
- the number of stripe: 1024.
- hash code A = 1
- hash code B = 1025.
compareTo will return non-zero but they are mapped to the same lock.
Comment on lines
+40
to
+42
| private final LockInfo volumeLock; | ||
| private final LockInfo bucketLock; | ||
| private final Set<LockInfo> keyLocks; |
There was a problem hiding this comment.
If we require locking on two or more volumes/buckets later on, it will require a lot of code change.
Comment on lines
+118
to
+135
| private void acquireLocks(List<Lock> locks) throws TimeoutException, InterruptedException { | ||
| List<Lock> acquiredLocks = new ArrayList<>(locks.size()); | ||
| for (Lock lock: locks) { | ||
| if (lock.tryLock(timeoutMillis, TimeUnit.MILLISECONDS)) { | ||
| try { | ||
| acquiredLocks.add(lock); | ||
| } catch (Throwable e) { | ||
| // We acquired this lock but were unable to add it to our acquired locks list. | ||
| lock.unlock(); | ||
| releaseLocks(acquiredLocks); | ||
| throw e; | ||
| } | ||
| } else { | ||
| releaseLocks(acquiredLocks); | ||
| throw new TimeoutException("Failed to acquire lock after the given timeout."); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Concurrently, if the lock.tryLock(..) is interrupted, the partial lock won't be released. Getting everything right with timeout is not easy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Prototype for the granular locking framework OBS implementation. The following aspects are considered in this design:
OmLockInfowill be consumed by all of the ~100 OM requests, so it should be as minimal as possible.OmRequestGatekeeperprovides one public method with a simple contract, and an initial implementation that is as simple as possible.Since this is prototype/demonstration code, see inline for comments on how the pieces will fit together.
What is the link to the Apache JIRA
HDDS-12356
How was this patch tested?
N/A, for demonstration and discussion only.