From 696a2a7315f54b5d605971e5ab5d722a9a4264d0 Mon Sep 17 00:00:00 2001 From: Tsz-Wo Nicholas Sze Date: Fri, 31 Jul 2026 13:56:07 -0700 Subject: [PATCH 1/4] HDDS-16059. Optimize varargs in IOzoneManagerLock. --- .../hadoop/hdds/utils/CompositeKey.java | 107 +++++++++---- .../hadoop/hdds/utils/TestCompositeKey.java | 108 +++++++++++++ .../ozone/om/lock/IOzoneManagerLock.java | 34 +++- .../ozone/om/lock/OBSKeyPathLockStrategy.java | 1 + .../hadoop/ozone/om/lock/OmReadOnlyLock.java | 48 +++++- .../ozone/om/lock/OzoneManagerLock.java | 140 ++++++++-------- .../hadoop/ozone/om/lock/TestKeyPathLock.java | 22 ++- .../ozone/om/lock/TestOzoneManagerLock.java | 150 +++++++++++------- .../ozone/om/snapshot/TestSnapshotCache.java | 16 ++ 9 files changed, 433 insertions(+), 193 deletions(-) create mode 100644 hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/TestCompositeKey.java diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java index 2f54ae7c7019..b9b859439d54 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java @@ -18,53 +18,92 @@ package org.apache.hadoop.hdds.utils; import java.util.Arrays; +import java.util.Objects; +import org.apache.ratis.util.Preconditions; /** * This is a utility to combine multiple objects as a key that can be used in * hash map access. The advantage of this is that it is cheap in comparison * to other methods like string concatenation. - * - * For example, if a composition of volume, bucket and key is needed to - * access a hash map, the natural method is: - * 
 {@code
- * String key = "/" + volume + "/" + bucket + "/" + key.
- * map.put(key, value);
- * }
- * This is costly because it creates (and stores) a new buffer. - * - * In comparison, the following achieve the same logic without creating any new - * buffer. - * 
 {@code
- * Object key = combineKeys(volume, bucket, key).
- * map.put(key, value);
- * }
- * */ -public final class CompositeKey { - private final int hashCode; - private final Object[] components; - - CompositeKey(Object[] components) { - this.components = components; - this.hashCode = Arrays.hashCode(components); +public abstract class CompositeKey { + /** The same as {@link Arrays#hashCode(Object[])} for one loop step. */ + static int hash(int result, Object next) { + return 31 * result + next.hashCode(); } - @Override - public int hashCode() { - return hashCode; + private static final class TwoComponents extends CompositeKey { + private final int hashCode; + private final Object first; + private final Object second; + + private TwoComponents(Object first, Object second) { + this.hashCode = hash(hash(1, first), second); + this.first = Objects.requireNonNull(first, "first == null"); + this.second = Objects.requireNonNull(second, "second == null"); + } + + @Override + public int hashCode() { + return hashCode; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } else if (!(obj instanceof TwoComponents)) { + return false; + } + final TwoComponents that = (TwoComponents) obj; + return Objects.equals(this.first, that.first) + && Objects.equals(this.second, that.second); + } } - @Override - public boolean equals(Object obj) { - if (!(obj instanceof CompositeKey)) { - return false; + private static class MultiComponents extends CompositeKey { + private final int hashCode; + private final Object[] components; + + MultiComponents(Object[] components) { + Preconditions.assertTrue(components.length > 2, () -> "components.length " + components.length + " <= 2"); + for (int i = 0; i < components.length; i++) { + final int j = i; + Objects.requireNonNull(components[j], () -> "components[" + j + "] == null"); + } + + this.hashCode = Arrays.hashCode(components); + this.components = components; + } + + @Override + public int hashCode() { + return hashCode; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } else if (!(obj instanceof MultiComponents)) { + return false; + } + final MultiComponents that = (MultiComponents) obj; + return Arrays.equals(this.components, that.components); } - CompositeKey other = (CompositeKey) obj; - return Arrays.equals(components, other.components); + } + + public static Object combineTwoKeys(Object first, Object second) { + return new TwoComponents(first, second); + } + + public static Object combineMultiKeys(Object[] components) { + return new MultiComponents(components); } public static Object combineKeys(Object[] components) { - return components.length == 1 ? - components[0] : new CompositeKey(components); + return components.length == 1 ? components[0] + : components.length == 2 ? CompositeKey.combineTwoKeys(components[0], components[1]) + : CompositeKey.combineMultiKeys(components); } } diff --git a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/TestCompositeKey.java b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/TestCompositeKey.java new file mode 100644 index 000000000000..c286d1e92590 --- /dev/null +++ b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/TestCompositeKey.java @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.hadoop.hdds.utils; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.util.Arrays; +import java.util.Random; +import org.junit.jupiter.api.Test; + +/** Test {@link CompositeKey}. */ +public final class TestCompositeKey { + private static final Random RANDOM = new Random(); + + static String randomString(int length) { + final StringBuilder builder = new StringBuilder(length); + for (int i = 0; i < length; i++) { + builder.append(RANDOM.nextInt(10)); + } + return builder.toString(); + } + + static Object[] randomComponents(int numComponents) { + final Object[] components = new Object[numComponents]; + for (int i = 0; i < components.length; i++) { + components[i] = randomString(RANDOM.nextInt(10)); + } + return components; + } + + private static final class OldCompositeKey { + private final int hashCode; + private final Object[] components; + + OldCompositeKey(Object[] components) { + this.components = components; + this.hashCode = Arrays.hashCode(components); + } + + @Override + public int hashCode() { + return hashCode; + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof OldCompositeKey)) { + return false; + } + OldCompositeKey other = (OldCompositeKey) obj; + return Arrays.equals(components, other.components); + } + + static Object combineKeys(Object[] components) { + return components.length == 1 ? + components[0] : new OldCompositeKey(components); + } + } + + static void assertHashCode(Object[] components, int computed) { + final Object expected = OldCompositeKey.combineKeys(components); + assertEquals(expected.hashCode(), CompositeKey.combineKeys(components).hashCode()); + assertEquals(expected.hashCode(), computed); + } + + @Test + public void testHashCodeOne() { + for (int i = 0; i < 100; i++) { + final Object[] components = {randomString(i)}; + assertHashCode(components, components[0].hashCode()); + } + } + + @Test + public void testHashCodeTwo() { + for (int i = 0; i < 10; i++) { + for (int j = 0; j < 10; j++) { + final Object first = randomString(i); + final Object second = randomString(j); + final Object[] components = {first, second}; + assertHashCode(components, CompositeKey.combineTwoKeys(first, second).hashCode()); + } + } + } + + @Test + public void testHashCodeMulti() { + for (int i = 3; i < 100; i++) { + final Object[] components = randomComponents(i); + assertHashCode(components, CompositeKey.combineMultiKeys(components).hashCode()); + } + } +} diff --git a/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/lock/IOzoneManagerLock.java b/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/lock/IOzoneManagerLock.java index e9319a787e7c..167a09d96fd3 100644 --- a/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/lock/IOzoneManagerLock.java +++ b/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/lock/IOzoneManagerLock.java @@ -26,34 +26,52 @@ */ public interface IOzoneManagerLock { - OMLockDetails acquireReadLock(Resource resource, - String... resources); + // ---------- acquireReadLock ---------- + OMLockDetails acquireReadLock(Resource resource, String key); + + OMLockDetails acquireReadLock(Resource resource, String key1, String key2); + + OMLockDetails acquireReadLock(Resource resource, String... keys); OMLockDetails acquireReadLocks(Resource resource, Iterable keys); - OMLockDetails acquireWriteLock(Resource resource, - String... resources); + // ---------- acquireWriteLock ---------- + OMLockDetails acquireWriteLock(Resource resource, String key); + + OMLockDetails acquireWriteLock(Resource resource, String key1, String key2); + + OMLockDetails acquireWriteLock(Resource resource, String... keys); OMLockDetails acquireWriteLocks(Resource resource, Iterable keys); OMLockDetails acquireResourceWriteLock(Resource resource); + // ---------- MultiUserLock ---------- boolean acquireMultiUserLock(String firstUser, String secondUser); void releaseMultiUserLock(String firstUser, String secondUser); - OMLockDetails releaseWriteLock(Resource resource, - String... resources); + // ---------- releaseWriteLock ---------- + OMLockDetails releaseWriteLock(Resource resource, String key); + + OMLockDetails releaseWriteLock(Resource resource, String key1, String key2); + + OMLockDetails releaseWriteLock(Resource resource, String... keys); OMLockDetails releaseWriteLocks(Resource resource, Iterable keys); OMLockDetails releaseResourceWriteLock(Resource resource); - OMLockDetails releaseReadLock(Resource resource, - String... resources); + // ---------- releaseReadLock ---------- + OMLockDetails releaseReadLock(Resource resource, String key); + + OMLockDetails releaseReadLock(Resource resource, String key1, String key2); + + OMLockDetails releaseReadLock(Resource resource, String... keys); OMLockDetails releaseReadLocks(Resource resource, Iterable keys); + // ---------- other methods ---------- @VisibleForTesting int getReadHoldCount(Resource resource, String... resources); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OBSKeyPathLockStrategy.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OBSKeyPathLockStrategy.java index c715856db80f..f444589af95f 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OBSKeyPathLockStrategy.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OBSKeyPathLockStrategy.java @@ -43,6 +43,7 @@ public OMLockDetails acquireWriteLock(OMMetadataManager omMetadataManager, Preconditions.checkArgument(omLockDetails.isLockAcquired(), "BUCKET_LOCK should be acquired!"); + // TODO optimize three key case in similar way as HDDS-16059 omLockDetails.merge(omMetadataManager.getLock() .acquireWriteLock(KEY_PATH_LOCK, volumeName, bucketName, keyName)); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmReadOnlyLock.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmReadOnlyLock.java index 7e544b010202..96f84219601f 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmReadOnlyLock.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmReadOnlyLock.java @@ -28,7 +28,17 @@ public class OmReadOnlyLock implements IOzoneManagerLock { @Override - public OMLockDetails acquireReadLock(Resource resource, String... resources) { + public OMLockDetails acquireReadLock(Resource resource, String key) { + return EMPTY_DETAILS_LOCK_ACQUIRED; + } + + @Override + public OMLockDetails acquireReadLock(Resource resource, String key1, String key2) { + return EMPTY_DETAILS_LOCK_ACQUIRED; + } + + @Override + public OMLockDetails acquireReadLock(Resource resource, String... keys) { return EMPTY_DETAILS_LOCK_ACQUIRED; } @@ -38,8 +48,17 @@ public OMLockDetails acquireReadLocks(Resource resource, Iterable keys } @Override - public OMLockDetails acquireWriteLock(Resource resource, - String... resources) { + public OMLockDetails acquireWriteLock(Resource resource, String key) { + return EMPTY_DETAILS_LOCK_NOT_ACQUIRED; + } + + @Override + public OMLockDetails acquireWriteLock(Resource resource, String key1, String key2) { + return EMPTY_DETAILS_LOCK_NOT_ACQUIRED; + } + + @Override + public OMLockDetails acquireWriteLock(Resource resource, String... keys) { return EMPTY_DETAILS_LOCK_NOT_ACQUIRED; } @@ -64,8 +83,17 @@ public void releaseMultiUserLock(String firstUser, String secondUser) { } @Override - public OMLockDetails releaseWriteLock(Resource resource, - String... resources) { + public OMLockDetails releaseWriteLock(Resource resource, String key) { + return EMPTY_DETAILS_LOCK_NOT_ACQUIRED; + } + + @Override + public OMLockDetails releaseWriteLock(Resource resource, String key1, String key2) { + return EMPTY_DETAILS_LOCK_NOT_ACQUIRED; + } + + @Override + public OMLockDetails releaseWriteLock(Resource resource, String... keys) { return EMPTY_DETAILS_LOCK_NOT_ACQUIRED; } @@ -79,6 +107,16 @@ public OMLockDetails releaseResourceWriteLock(Resource resource) { return EMPTY_DETAILS_LOCK_NOT_ACQUIRED; } + @Override + public OMLockDetails releaseReadLock(Resource resource, String key) { + return EMPTY_DETAILS_LOCK_NOT_ACQUIRED; + } + + @Override + public OMLockDetails releaseReadLock(Resource resource, String key1, String key2) { + return EMPTY_DETAILS_LOCK_NOT_ACQUIRED; + } + @Override public OMLockDetails releaseReadLock(Resource resource, String... resources) { return EMPTY_DETAILS_LOCK_NOT_ACQUIRED; diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java index a6a47844519b..588bd5dd6afe 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java @@ -124,11 +124,14 @@ R assertAcquire(Resource resource) { private ReentrantReadWriteLock getLockForTesting(Resource resource, String... keys) { final R r = Preconditions.assertInstanceOf(resource, tracker.getResourceClass()); - return getLock(r, keys); + final Object combinedKey = keys.length == 1 ? keys[0] + : keys.length == 2 ? CompositeKey.combineTwoKeys(keys[0], keys[1]) + : CompositeKey.combineMultiKeys(keys); + return getLockWithCombinedKey(r, combinedKey); } - private ReentrantReadWriteLock getLock(R r, String... keys) { - return lockMap.get(r).get(CompositeKey.combineKeys(keys)); + private ReentrantReadWriteLock getLockWithCombinedKey(R r, Object combinedKey) { + return lockMap.get(r).get(combinedKey); } private void acquireLock(R resource, boolean isRead, ReentrantReadWriteLock lock, long startWaitingTimeNanos) { @@ -148,9 +151,9 @@ private OMLockDetails acquireImpl(Resource resource, BiConsumer acquire return tracker.lockResource(r); } - private OMLockDetails acquireOne(Resource resource, boolean isRead, String... keys) { + private OMLockDetails acquireOne(Resource resource, boolean isRead, Object combinedKey) { return acquireImpl(resource, (r, startWaitingTimeNanos) -> { - final ReentrantReadWriteLock lock = getLock(r, keys); + final ReentrantReadWriteLock lock = getLockWithCombinedKey(r, combinedKey); acquireLock(r, isRead, lock, startWaitingTimeNanos); }); } @@ -190,9 +193,9 @@ private OMLockDetails releaseImpl(Resource resource, Consumer releaseLockMeth return tracker.unlockResource(r); } - private OMLockDetails releaseOne(Resource resource, boolean isRead, String... keys) { + private OMLockDetails releaseOne(Resource resource, boolean isRead, Object combinedKey) { return releaseImpl(resource, r -> { - final ReentrantReadWriteLock lock = getLock(r, keys); + final ReentrantReadWriteLock lock = getLockWithCombinedKey(r, combinedKey); releaseLock(r, isRead, lock); }); } @@ -305,73 +308,47 @@ public ReentrantReadWriteLock next() { return deque::descendingIterator; } - /** - * Acquire read lock on resource. - * - * For S3_BUCKET_LOCK, VOLUME_LOCK, BUCKET_LOCK type resource, same - * thread acquiring lock again is allowed. - * - * For USER_LOCK, PREFIX_LOCK, S3_SECRET_LOCK type resource, same thread - * acquiring lock again is not allowed. - * - * Special Note for USER_LOCK: Single thread can acquire single user lock/ - * multi user lock. But not both at the same time. - * @param resource - Type of the resource. - * @param keys - Resource names on which user want to acquire lock. - * For Resource type BUCKET_LOCK, first param should be volume, second param - * should be bucket name. For remaining all resource only one param should - * be passed. - */ + @Override + public OMLockDetails acquireReadLock(Resource resource, String key) { + return getResourceLocks(resource) + .acquireOne(resource, true, key); + } + + @Override + public OMLockDetails acquireReadLock(Resource resource, String key1, String key2) { + return getResourceLocks(resource) + .acquireOne(resource, true, CompositeKey.combineTwoKeys(key1, key2)); + } + @Override public OMLockDetails acquireReadLock(Resource resource, String... keys) { + Preconditions.assertTrue(keys.length > 2); return getResourceLocks(resource) - .acquireOne(resource, true, keys); + .acquireOne(resource, true, CompositeKey.combineMultiKeys(keys)); } - /** - * Acquire read locks on a list of resources. - * - * For S3_BUCKET_LOCK, VOLUME_LOCK, BUCKET_LOCK type resource, same - * thread acquiring lock again is allowed. - * - * For USER_LOCK, PREFIX_LOCK, S3_SECRET_LOCK type resource, same thread - * acquiring lock again is not allowed. - * - * Special Note for USER_LOCK: Single thread can acquire single user lock/ - * multi user lock. But not both at the same time. - * @param resource - Type of the resource. - * @param keys - A list of Resource names on which user want to acquire locks. - * For Resource type BUCKET_LOCK, first param should be volume, second param - * should be bucket name. For remaining all resource only one param should - * be passed. - */ @Override public OMLockDetails acquireReadLocks(Resource resource, Iterable keys) { return getResourceLocks(resource) .acquireSelected(resource, true, keys); } - /** - * Acquire write lock on resource. - * - * For S3_BUCKET_LOCK, VOLUME_LOCK, BUCKET_LOCK type resource, same - * thread acquiring lock again is allowed. - * - * For USER_LOCK, PREFIX_LOCK, S3_SECRET_LOCK type resource, same thread - * acquiring lock again is not allowed. - * - * Special Note for USER_LOCK: Single thread can acquire single user lock/ - * multi user lock. But not both at the same time. - * @param resource - Type of the resource. - * @param keys - Resource names on which user want to acquire lock. - * For Resource type BUCKET_LOCK, first param should be volume, second param - * should be bucket name. For remaining all resource only one param should - * be passed. - */ + @Override + public OMLockDetails acquireWriteLock(Resource resource, String key) { + return getResourceLocks(resource) + .acquireOne(resource, false, key); + } + + @Override + public OMLockDetails acquireWriteLock(Resource resource, String key1, String key2) { + return getResourceLocks(resource) + .acquireOne(resource, false, CompositeKey.combineTwoKeys(key1, key2)); + } + @Override public OMLockDetails acquireWriteLock(Resource resource, String... keys) { return getResourceLocks(resource) - .acquireOne(resource, false, keys); + .acquireOne(resource, false, CompositeKey.combineMultiKeys(keys)); } /** @@ -474,19 +451,22 @@ public void releaseMultiUserLock(String firstUser, String secondUser) { Arrays.asList(new String[] {firstUser}, new String[] {secondUser})); } + @Override + public OMLockDetails releaseWriteLock(Resource resource, String key) { + return getResourceLocks(resource) + .releaseOne(resource, false, key); + } + + @Override + public OMLockDetails releaseWriteLock(Resource resource, String key1, String key2) { + return getResourceLocks(resource) + .releaseOne(resource, false, CompositeKey.combineTwoKeys(key1, key2)); + } - /** - * Release write lock on resource. - * @param resource - Type of the resource. - * @param keys - Resource names on which user want to acquire lock. - * For Resource type BUCKET_LOCK, first param should be volume, second param - * should be bucket name. For remaining all resource only one param should - * be passed. - */ @Override public OMLockDetails releaseWriteLock(Resource resource, String... keys) { return getResourceLocks(resource) - .releaseOne(resource, false, keys); + .releaseOne(resource, false, CompositeKey.combineMultiKeys(keys)); } /** @@ -514,18 +494,22 @@ public OMLockDetails releaseResourceWriteLock(Resource resource) { .releaseAll(resource); } - /** - * Release read lock on resource. - * @param resource - Type of the resource. - * @param keys - Resource names on which user want to acquire lock. - * For Resource type BUCKET_LOCK, first param should be volume, second param - * should be bucket name. For remaining all resource only one param should - * be passed. - */ + @Override + public OMLockDetails releaseReadLock(Resource resource, String key) { + return getResourceLocks(resource) + .releaseOne(resource, true, key); + } + + @Override + public OMLockDetails releaseReadLock(Resource resource, String key1, String key2) { + return getResourceLocks(resource) + .releaseOne(resource, true, CompositeKey.combineTwoKeys(key1, key2)); + } + @Override public OMLockDetails releaseReadLock(Resource resource, String... keys) { return getResourceLocks(resource) - .releaseOne(resource, true, keys); + .releaseOne(resource, true, CompositeKey.combineMultiKeys(keys)); } /** diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/lock/TestKeyPathLock.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/lock/TestKeyPathLock.java index 77b7999d6162..eab2bd596645 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/lock/TestKeyPathLock.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/lock/TestKeyPathLock.java @@ -233,12 +233,11 @@ void testAcquireWriteBucketLockWhileAcquiredWriteKeyPathLock() { OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); - String[] resourceName = new String[]{volumeName, bucketName, keyName}, - higherResourceName = new String[]{volumeName, bucketName}; + String[] resourceName = new String[]{volumeName, bucketName, keyName}; lock.acquireWriteLock(resource, resourceName); RuntimeException ex = - assertThrows(RuntimeException.class, () -> lock.acquireWriteLock(higherResource, higherResourceName)); + assertThrows(RuntimeException.class, () -> lock.acquireWriteLock(higherResource, volumeName, bucketName)); String message = "cannot acquire " + higherResource.getName() + " lock " + "while holding [" + resource.getName() + "] lock(s)."; assertThat(ex).hasMessageContaining(message); @@ -255,12 +254,11 @@ void testAcquireWriteBucketLockWhileAcquiredReadKeyPathLock() { OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); - String[] resourceName = new String[]{volumeName, bucketName, keyName}, - higherResourceName = new String[]{volumeName, bucketName}; + String[] resourceName = new String[]{volumeName, bucketName, keyName}; lock.acquireReadLock(resource, resourceName); - RuntimeException ex = - assertThrows(RuntimeException.class, () -> lock.acquireWriteLock(higherResource, higherResourceName)); + RuntimeException ex = assertThrows(RuntimeException.class, + () -> lock.acquireWriteLock(higherResource, volumeName, bucketName)); String message = "cannot acquire " + higherResource.getName() + " lock " + "while holding [" + resource.getName() + "] lock(s)."; assertThat(ex).hasMessageContaining(message); @@ -277,12 +275,11 @@ void testAcquireReadBucketLockWhileAcquiredReadKeyPathLock() { OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); - String[] resourceName = new String[]{volumeName, bucketName, keyName}, - higherResourceName = new String[]{volumeName, bucketName}; + String[] resourceName = new String[]{volumeName, bucketName, keyName}; lock.acquireReadLock(resource, resourceName); RuntimeException ex = - assertThrows(RuntimeException.class, () -> lock.acquireReadLock(higherResource, higherResourceName)); + assertThrows(RuntimeException.class, () -> lock.acquireReadLock(higherResource, volumeName, bucketName)); String message = "cannot acquire " + higherResource.getName() + " lock " + "while holding [" + resource.getName() + "] lock(s)."; assertThat(ex).hasMessageContaining(message); @@ -299,12 +296,11 @@ void testAcquireReadBucketLockWhileAcquiredWriteKeyPathLock() { OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); - String[] resourceName = new String[]{volumeName, bucketName, keyName}, - higherResourceName = new String[]{volumeName, bucketName}; + String[] resourceName = new String[]{volumeName, bucketName, keyName}; lock.acquireWriteLock(resource, resourceName); RuntimeException ex = - assertThrows(RuntimeException.class, () -> lock.acquireReadLock(higherResource, higherResourceName)); + assertThrows(RuntimeException.class, () -> lock.acquireReadLock(higherResource, volumeName, bucketName)); String message = "cannot acquire " + higherResource.getName() + " lock " + "while holding [" + resource.getName() + "] lock(s)."; assertThat(ex).hasMessageContaining(message); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java index 5aaab032b32f..3f275671eb61 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java @@ -67,8 +67,8 @@ void acquireResourceLock(LeveledResource resource) { private void testResourceLock(String[] resourceName, LeveledResource resource) { OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); - lock.acquireWriteLock(resource, resourceName); - assertDoesNotThrow(() -> lock.releaseWriteLock(resource, resourceName)); + acquireWriteLock(lock, resource, resourceName); + assertDoesNotThrow(() -> releaseWriteLock(lock, resource, resourceName)); } @ParameterizedTest @@ -86,18 +86,18 @@ private void testResourceReacquireLock(String[] resourceName, if (resource == LeveledResource.USER_LOCK || resource == LeveledResource.S3_SECRET_LOCK || resource == LeveledResource.PREFIX_LOCK) { - lock.acquireWriteLock(resource, resourceName); + acquireWriteLock(lock, resource, resourceName); RuntimeException ex = - assertThrows(RuntimeException.class, () -> lock.acquireWriteLock(resource, resourceName)); + assertThrows(RuntimeException.class, () -> acquireWriteLock(lock, resource, resourceName)); String message = "cannot acquire " + resource.getName() + " lock " + "while holding [" + resource.getName() + "] lock(s)."; assertThat(ex).hasMessageContaining(message); - assertDoesNotThrow(() -> lock.releaseWriteLock(resource, resourceName)); + assertDoesNotThrow(() -> releaseWriteLock(lock, resource, resourceName)); } else { - lock.acquireWriteLock(resource, resourceName); - lock.acquireWriteLock(resource, resourceName); - assertDoesNotThrow(() -> lock.releaseWriteLock(resource, resourceName)); - assertDoesNotThrow(() -> lock.releaseWriteLock(resource, resourceName)); + acquireWriteLock(lock, resource, resourceName); + acquireWriteLock(lock, resource, resourceName); + assertDoesNotThrow(() -> releaseWriteLock(lock, resource, resourceName)); + assertDoesNotThrow(() -> releaseWriteLock(lock, resource, resourceName)); } } @@ -112,12 +112,12 @@ void testLockingOrder() { for (LeveledResource resource : LeveledResource.values()) { Stack stack = new Stack<>(); resourceName = generateResourceName(resource); - lock.acquireWriteLock(resource, resourceName); + acquireWriteLock(lock, resource, resourceName); stack.push(new ResourceInfo(resourceName, resource)); for (LeveledResource higherResource : LeveledResource.values()) { if (higherResource.getMask() > resource.getMask()) { resourceName = generateResourceName(higherResource); - lock.acquireWriteLock(higherResource, resourceName); + acquireWriteLock(lock, higherResource, resourceName); stack.push(new ResourceInfo(resourceName, higherResource)); } } @@ -125,7 +125,7 @@ void testLockingOrder() { while (!stack.empty()) { ResourceInfo resourceInfo = stack.pop(); assertDoesNotThrow(() -> - lock.releaseWriteLock(resourceInfo.getResource(), resourceInfo.getLockName())); + releaseWriteLock(lock, resourceInfo.getResource(), resourceInfo.getLockName())); } } } @@ -144,19 +144,19 @@ public void testDAGLockWithParallelResource(DAGLeveledResource dagLeveledResourc for (Resource otherResource : resources) { String[] otherResourceName = generateResourceName(otherResource); String[] dagResourceName = generateResourceName(dagLeveledResource); - lock.acquireWriteLock(otherResource, otherResourceName); + acquireWriteLock(lock, otherResource, otherResourceName); boolean secondLockAcquired = false; try { if (forbiddenLockOrdering.getOrDefault(dagLeveledResource, Collections.emptySet()).contains(otherResource)) { - assertThrows(RuntimeException.class, () -> lock.acquireWriteLock(dagLeveledResource, dagResourceName)); + assertThrows(RuntimeException.class, () -> acquireWriteLock(lock, dagLeveledResource, dagResourceName)); } else { - lock.acquireWriteLock(dagLeveledResource, dagResourceName); + acquireWriteLock(lock, dagLeveledResource, dagResourceName); secondLockAcquired = true; } } finally { - lock.releaseWriteLock(otherResource, otherResourceName); + releaseWriteLock(lock, otherResource, otherResourceName); if (secondLockAcquired) { - lock.releaseWriteLock(dagLeveledResource, dagResourceName); + releaseWriteLock(lock, dagLeveledResource, dagResourceName); } } } @@ -169,15 +169,15 @@ void testLockViolationsWithOneHigherLevelLock(LeveledResource resource) { for (LeveledResource higherResource : LeveledResource.values()) { if (higherResource.getMask() > resource.getMask()) { String[] resourceName = generateResourceName(higherResource); - lock.acquireWriteLock(higherResource, resourceName); + acquireWriteLock(lock, higherResource, resourceName); try { Exception e = assertThrows(RuntimeException.class, - () -> lock.acquireWriteLock(resource, generateResourceName(resource))); + () -> acquireWriteLock(lock, resource, generateResourceName(resource))); String message = "cannot acquire " + resource.getName() + " lock " + "while holding [" + higherResource.getName() + "] lock(s)."; assertThat(e).hasMessageContaining(message); } finally { - lock.releaseWriteLock(higherResource, resourceName); + releaseWriteLock(lock, higherResource, resourceName); } } } @@ -197,13 +197,13 @@ void testLockViolations() { for (LeveledResource higherResource : LeveledResource.values()) { if (higherResource.getMask() > resource.getMask()) { resourceName = generateResourceName(higherResource); - lock.acquireWriteLock(higherResource, resourceName); + acquireWriteLock(lock, higherResource, resourceName); stack.push(new ResourceInfo(resourceName, higherResource)); currentLocks.add(higherResource.getName()); // try to acquire lower level lock RuntimeException ex = assertThrows(RuntimeException.class, () -> { String[] resourceName1 = generateResourceName(resource); - lock.acquireWriteLock(resource, resourceName1); + acquireWriteLock(lock, resource, resourceName1); }); String message = "cannot acquire " + resource.getName() + " lock " + "while holding " + currentLocks + " lock(s)."; @@ -214,7 +214,7 @@ void testLockViolations() { // Now release locks while (!stack.empty()) { ResourceInfo resourceInfo = stack.pop(); - lock.releaseWriteLock(resourceInfo.getResource(), + releaseWriteLock(lock, resourceInfo.getResource(), resourceInfo.getLockName()); } } @@ -225,7 +225,7 @@ void releaseLockWithOutAcquiringLock() { OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); assertThrows(IllegalMonitorStateException.class, - () -> lock.releaseWriteLock(LeveledResource.USER_LOCK, "user3")); + () -> releaseWriteLock(lock, LeveledResource.USER_LOCK, "user3")); } private String[] generateResourceName(Resource resource) { @@ -241,6 +241,46 @@ private String[] generateResourceName(Resource resource) { } } + static void acquireReadLock(OzoneManagerLock lock, Resource resource, String... keys) { + if (keys.length == 1) { + lock.acquireReadLock(resource, keys[0]); + } else if (keys.length == 2) { + lock.acquireReadLock(resource, keys[0], keys[1]); + } else { + lock.acquireReadLock(resource, keys); + } + } + + static void acquireWriteLock(OzoneManagerLock lock, Resource resource, String... keys) { + if (keys.length == 1) { + lock.acquireWriteLock(resource, keys[0]); + } else if (keys.length == 2) { + lock.acquireWriteLock(resource, keys[0], keys[1]); + } else { + lock.acquireWriteLock(resource, keys); + } + } + + static void releaseWriteLock(OzoneManagerLock lock, Resource resource, String... keys) { + if (keys.length == 1) { + lock.releaseWriteLock(resource, keys[0]); + } else if (keys.length == 2) { + lock.releaseWriteLock(resource, keys[0], keys[1]); + } else { + lock.releaseWriteLock(resource, keys); + } + } + + static void releaseReadLock(OzoneManagerLock lock, Resource resource, String... keys) { + if (keys.length == 1) { + lock.releaseReadLock(resource, keys[0]); + } else if (keys.length == 2) { + lock.releaseReadLock(resource, keys[0], keys[1]); + } else { + lock.releaseReadLock(resource, keys); + } + } + /** * Class used to store locked resource info. */ @@ -283,12 +323,12 @@ void reAcquireMultiUserLock() { @Test void acquireMultiUserLockAfterUserLock() { OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); - lock.acquireWriteLock(LeveledResource.USER_LOCK, "user3"); + acquireWriteLock(lock, LeveledResource.USER_LOCK, "user3"); Exception e = assertThrows(RuntimeException.class, () -> lock.acquireMultiUserLock("user1", "user2")); assertThat(e) .hasMessageContaining("cannot acquire USER_LOCK lock while holding [USER_LOCK] lock(s)."); - lock.releaseWriteLock(LeveledResource.USER_LOCK, "user3"); + releaseWriteLock(lock, LeveledResource.USER_LOCK, "user3"); } @Test @@ -296,7 +336,7 @@ void acquireUserLockAfterMultiUserLock() { OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); lock.acquireMultiUserLock("user1", "user2"); Exception e = assertThrows(RuntimeException.class, - () -> lock.acquireWriteLock(LeveledResource.USER_LOCK, "user3")); + () -> acquireWriteLock(lock, LeveledResource.USER_LOCK, "user3")); assertThat(e) .hasMessageContaining("cannot acquire USER_LOCK lock while holding [USER_LOCK] lock(s)."); lock.releaseMultiUserLock("user1", "user2"); @@ -313,7 +353,7 @@ void testLockResourceParallel(boolean fullResourceLock) throws Exception { if (fullResourceLock) { lock.acquireResourceWriteLock(resource); } else { - lock.acquireWriteLock(resource, resourceName); + acquireWriteLock(lock, resource, resourceName); } AtomicBoolean gotLock = new AtomicBoolean(false); @@ -321,13 +361,13 @@ void testLockResourceParallel(boolean fullResourceLock) throws Exception { if (fullResourceLock) { lock.acquireResourceWriteLock(resource); } else { - lock.acquireWriteLock(resource, resourceName); + acquireWriteLock(lock, resource, resourceName); } gotLock.set(true); if (fullResourceLock) { lock.releaseResourceWriteLock(resource); } else { - lock.releaseWriteLock(resource, resourceName); + releaseWriteLock(lock, resource, resourceName); } }).start(); @@ -339,7 +379,7 @@ void testLockResourceParallel(boolean fullResourceLock) throws Exception { if (fullResourceLock) { lock.releaseResourceWriteLock(resource); } else { - lock.releaseWriteLock(resource, resourceName); + releaseWriteLock(lock, resource, resourceName); } // Since we have released the lock, the new thread should have the lock // now. @@ -367,9 +407,9 @@ void testResourceLockFullResourceLockParallel(boolean mainThreadAcquireResourceL lock.acquireResourceWriteLock(resource); } else { if (acquireWriteLock) { - lock.acquireWriteLock(resource, resourceName); + acquireWriteLock(lock, resource, resourceName); } else { - lock.acquireReadLock(resource, resourceName); + acquireReadLock(lock, resource, resourceName); } } @@ -379,9 +419,9 @@ void testResourceLockFullResourceLockParallel(boolean mainThreadAcquireResourceL lock.acquireResourceWriteLock(resource); } else { if (acquireWriteLock) { - lock.acquireWriteLock(resource, resourceName); + acquireWriteLock(lock, resource, resourceName); } else { - lock.acquireReadLock(resource, resourceName); + acquireReadLock(lock, resource, resourceName); } } gotLock.set(true); @@ -389,9 +429,9 @@ void testResourceLockFullResourceLockParallel(boolean mainThreadAcquireResourceL lock.releaseResourceWriteLock(resource); } else { if (acquireWriteLock) { - lock.releaseWriteLock(resource, resourceName); + releaseWriteLock(lock, resource, resourceName); } else { - lock.releaseReadLock(resource, resourceName); + releaseReadLock(lock, resource, resourceName); } } }).start(); @@ -404,9 +444,9 @@ void testResourceLockFullResourceLockParallel(boolean mainThreadAcquireResourceL lock.releaseResourceWriteLock(resource); } else { if (acquireWriteLock) { - lock.releaseWriteLock(resource, resourceName); + releaseWriteLock(lock, resource, resourceName); } else { - lock.releaseReadLock(resource, resourceName); + releaseReadLock(lock, resource, resourceName); } } // Since we have released the lock, the new thread should have the lock @@ -486,33 +526,33 @@ private void testLockHoldCountUtil(LeveledResource resource, OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); assertEquals(0, lock.getReadHoldCount(resource, resourceName)); - lock.acquireReadLock(resource, resourceName); + acquireReadLock(lock, resource, resourceName); assertEquals(1, lock.getReadHoldCount(resource, resourceName)); - lock.acquireReadLock(resource, resourceName); + acquireReadLock(lock, resource, resourceName); assertEquals(2, lock.getReadHoldCount(resource, resourceName)); - lock.releaseReadLock(resource, resourceName); + releaseReadLock(lock, resource, resourceName); assertEquals(1, lock.getReadHoldCount(resource, resourceName)); - lock.releaseReadLock(resource, resourceName); + releaseReadLock(lock, resource, resourceName); assertEquals(0, lock.getReadHoldCount(resource, resourceName)); assertFalse(lock.isWriteLockedByCurrentThread(resource, resourceName)); assertEquals(0, lock.getWriteHoldCount(resource, resourceName)); - lock.acquireWriteLock(resource, resourceName); + acquireWriteLock(lock, resource, resourceName); assertTrue(lock.isWriteLockedByCurrentThread(resource, resourceName)); assertEquals(1, lock.getWriteHoldCount(resource, resourceName)); - lock.acquireWriteLock(resource, resourceName); + acquireWriteLock(lock, resource, resourceName); assertTrue(lock.isWriteLockedByCurrentThread(resource, resourceName)); assertEquals(2, lock.getWriteHoldCount(resource, resourceName)); - lock.releaseWriteLock(resource, resourceName); + releaseWriteLock(lock, resource, resourceName); assertTrue(lock.isWriteLockedByCurrentThread(resource, resourceName)); assertEquals(1, lock.getWriteHoldCount(resource, resourceName)); - lock.releaseWriteLock(resource, resourceName); + releaseWriteLock(lock, resource, resourceName); assertFalse(lock.isWriteLockedByCurrentThread(resource, resourceName)); assertEquals(0, lock.getWriteHoldCount(resource, resourceName)); } @@ -535,13 +575,13 @@ private void testReadLockConcurrentStats(LeveledResource resource, for (int i = 0; i < threads.length; i++) { threads[i] = new Thread(() -> { - lock.acquireReadLock(resource, resourceName); + acquireReadLock(lock, resource, resourceName); try { Thread.sleep(500); } catch (InterruptedException e) { e.printStackTrace(); } - lock.releaseReadLock(resource, resourceName); + releaseReadLock(lock, resource, resourceName); }); threads[i].start(); } @@ -567,13 +607,13 @@ private void testWriteLockConcurrentStats(LeveledResource resource, for (int i = 0; i < threads.length; i++) { threads[i] = new Thread(() -> { - lock.acquireWriteLock(resource, resourceName); + acquireWriteLock(lock, resource, resourceName); try { Thread.sleep(100); } catch (InterruptedException e) { e.printStackTrace(); } - lock.releaseWriteLock(resource, resourceName); + releaseWriteLock(lock, resource, resourceName); }); threads[i].start(); } @@ -600,13 +640,13 @@ private void testSyntheticReadWriteLockConcurrentStats( for (int i = 0; i < readThreads.length; i++) { readThreads[i] = new Thread(() -> { - lock.acquireReadLock(resource, resourceName); + acquireReadLock(lock, resource, resourceName); try { Thread.sleep(500); } catch (InterruptedException e) { e.printStackTrace(); } - lock.releaseReadLock(resource, resourceName); + releaseReadLock(lock, resource, resourceName); }); readThreads[i].setName("ReadLockThread-" + i); readThreads[i].start(); @@ -614,13 +654,13 @@ private void testSyntheticReadWriteLockConcurrentStats( for (int i = 0; i < writeThreads.length; i++) { writeThreads[i] = new Thread(() -> { - lock.acquireWriteLock(resource, resourceName); + acquireWriteLock(lock, resource, resourceName); try { Thread.sleep(100); } catch (InterruptedException e) { e.printStackTrace(); } - lock.releaseWriteLock(resource, resourceName); + releaseWriteLock(lock, resource, resourceName); }); writeThreads[i].setName("WriteLockThread-" + i); writeThreads[i].start(); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java index 96fdb35d4679..2c789729c063 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java @@ -518,16 +518,32 @@ void testSnapshotOperationsNotBlockedDuringCompaction() throws IOException, Inte private static IOzoneManagerLock newAcquiringLock() { IOzoneManagerLock acquiringLock = mock(IOzoneManagerLock.class); + when(acquiringLock.acquireReadLock(eq(SNAPSHOT_DB_LOCK), any(String.class))) + .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED); + when(acquiringLock.acquireReadLock(eq(SNAPSHOT_DB_LOCK), any(String.class), any(String.class))) + .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED); when(acquiringLock.acquireReadLock(eq(SNAPSHOT_DB_LOCK), any(String[].class))) .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED); + when(acquiringLock.releaseReadLock(eq(SNAPSHOT_DB_LOCK), any(String.class))) + .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED); + when(acquiringLock.releaseReadLock(eq(SNAPSHOT_DB_LOCK), any(String.class), any(String.class))) + .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED); when(acquiringLock.releaseReadLock(eq(SNAPSHOT_DB_LOCK), any(String[].class))) .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED); when(acquiringLock.acquireResourceWriteLock(eq(SNAPSHOT_DB_LOCK))) .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED); when(acquiringLock.releaseResourceWriteLock(eq(SNAPSHOT_DB_LOCK))) .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED); + when(acquiringLock.acquireWriteLock(eq(SNAPSHOT_DB_LOCK), any(String.class))) + .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED); + when(acquiringLock.acquireWriteLock(eq(SNAPSHOT_DB_LOCK), any(String.class), any(String.class))) + .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED); when(acquiringLock.acquireWriteLock(eq(SNAPSHOT_DB_LOCK), any(String[].class))) .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED); + when(acquiringLock.releaseWriteLock(eq(SNAPSHOT_DB_LOCK), any(String.class))) + .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED); + when(acquiringLock.releaseWriteLock(eq(SNAPSHOT_DB_LOCK), any(String.class), any(String.class))) + .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED); when(acquiringLock.releaseWriteLock(eq(SNAPSHOT_DB_LOCK), any(String[].class))) .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED); return acquiringLock; From bb069308a479b794200ef73809cb56a0a55eca3a Mon Sep 17 00:00:00 2001 From: Tsz-Wo Nicholas Sze Date: Sat, 1 Aug 2026 08:24:28 -0700 Subject: [PATCH 2/4] Minor improvement --- .../org/apache/hadoop/hdds/utils/CompositeKey.java | 10 ++++++---- .../apache/hadoop/ozone/om/lock/OzoneManagerLock.java | 5 +---- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java index b9b859439d54..6d8aca503815 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java @@ -56,7 +56,8 @@ public boolean equals(Object obj) { return false; } final TwoComponents that = (TwoComponents) obj; - return Objects.equals(this.first, that.first) + return this.hashCode == that.hashCode + && Objects.equals(this.first, that.first) && Objects.equals(this.second, that.second); } } @@ -89,15 +90,16 @@ public boolean equals(Object obj) { return false; } final MultiComponents that = (MultiComponents) obj; - return Arrays.equals(this.components, that.components); + return this.hashCode == that.hashCode + && Arrays.equals(this.components, that.components); } } - public static Object combineTwoKeys(Object first, Object second) { + public static CompositeKey combineTwoKeys(Object first, Object second) { return new TwoComponents(first, second); } - public static Object combineMultiKeys(Object[] components) { + public static CompositeKey combineMultiKeys(Object[] components) { return new MultiComponents(components); } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java index 588bd5dd6afe..1847dd31b7cc 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java @@ -124,10 +124,7 @@ R assertAcquire(Resource resource) { private ReentrantReadWriteLock getLockForTesting(Resource resource, String... keys) { final R r = Preconditions.assertInstanceOf(resource, tracker.getResourceClass()); - final Object combinedKey = keys.length == 1 ? keys[0] - : keys.length == 2 ? CompositeKey.combineTwoKeys(keys[0], keys[1]) - : CompositeKey.combineMultiKeys(keys); - return getLockWithCombinedKey(r, combinedKey); + return getLockWithCombinedKey(r, CompositeKey.combineKeys(keys)); } private ReentrantReadWriteLock getLockWithCombinedKey(R r, Object combinedKey) { From 82aec180ccadc5a056b769debb12d69b6ed08608 Mon Sep 17 00:00:00 2001 From: Tsz-Wo Nicholas Sze Date: Sat, 1 Aug 2026 15:57:01 -0700 Subject: [PATCH 3/4] Address review comments --- .../java/org/apache/hadoop/hdds/utils/CompositeKey.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java index 6d8aca503815..5fb0fda286c2 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java @@ -57,12 +57,12 @@ public boolean equals(Object obj) { } final TwoComponents that = (TwoComponents) obj; return this.hashCode == that.hashCode - && Objects.equals(this.first, that.first) - && Objects.equals(this.second, that.second); + && this.first.equals(that.first) + && this.second.equals(that.second); } } - private static class MultiComponents extends CompositeKey { + private static final class MultiComponents extends CompositeKey { private final int hashCode; private final Object[] components; From ef8493634c53577f28b688a1d51b22b3fcddc23b Mon Sep 17 00:00:00 2001 From: Tsz-Wo Nicholas Sze Date: Sun, 2 Aug 2026 09:27:40 -0700 Subject: [PATCH 4/4] Check null first. --- .../main/java/org/apache/hadoop/hdds/utils/CompositeKey.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java index 5fb0fda286c2..ab8b244f1bfe 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CompositeKey.java @@ -38,9 +38,9 @@ private static final class TwoComponents extends CompositeKey { private final Object second; private TwoComponents(Object first, Object second) { - this.hashCode = hash(hash(1, first), second); this.first = Objects.requireNonNull(first, "first == null"); this.second = Objects.requireNonNull(second, "second == null"); + this.hashCode = hash(hash(1, first), second); } @Override