From 787fcc33c125dde5894f64c8c4a22d9ef9c3fd2c Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Fri, 21 Jun 2019 17:09:09 -0700 Subject: [PATCH 01/10] HDDS-1723. Create new OzoneManagerLock class. --- .../org/apache/hadoop/ozone/OzoneConsts.java | 1 + .../apache/hadoop/ozone/lock/LockManager.java | 5 +- .../ozone/om/lock/OzoneManagerLock.java | 288 ++++++++++++++ .../ozone/om/lock/OzoneManagerLockUtil.java | 55 +++ .../hadoop/ozone/om/lock/package-info.java | 22 + .../ozone/om/lock/TestOzoneManagerLock.java | 375 ++++++++++++++++++ .../hadoop/ozone/om/lock/package-info.java | 21 + 7 files changed, 765 insertions(+), 2 deletions(-) create mode 100644 hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java create mode 100644 hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java create mode 100644 hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/package-info.java create mode 100644 hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java create mode 100644 hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/package-info.java diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java index d2d80b928af7e..42ce374e64af4 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java @@ -170,6 +170,7 @@ public static Versioning getVersioning(boolean versioning) { public static final String OM_USER_PREFIX = "$"; public static final String OM_S3_PREFIX ="S3:"; public static final String OM_S3_VOLUME_PREFIX = "s3"; + public static final String OM_S3_SECRET = "S3Secret:"; /** * Max chunk size limit. diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/lock/LockManager.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/lock/LockManager.java index 49cf544626b7e..5f76bd6263ae9 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/lock/LockManager.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/lock/LockManager.java @@ -83,9 +83,10 @@ public void unlock(T resource) { ActiveLock lock = activeLocks.get(resource); if (lock == null) { // Someone is releasing a lock which was never acquired. Log and return. - LOG.warn("Trying to release the lock on {}, which was never acquired.", + LOG.error("Trying to release the lock on {}, which was never acquired.", resource); - return; + throw new IllegalMonitorStateException("Releasing lock on resource " + + resource + " without acquiring lock"); } lock.unlock(); activeLocks.computeIfPresent(resource, (k, v) -> { diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java new file mode 100644 index 0000000000000..5b71cb455cb98 --- /dev/null +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java @@ -0,0 +1,288 @@ +package org.apache.hadoop.ozone.om.lock; + + +import java.util.ArrayList; +import java.util.List; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.ozone.lock.LockManager; + +/** + * Provides different locks to handle concurrency in OzoneMaster. + * We also maintain lock hierarchy, based on the weight. + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + *
WEIGHT LOCK
0 S3 Bucket Lock
1 Volume Lock
2 Bucket Lock
3 User Lock
4 S3 Secret Lock
5 Prefix Lock
+ * + * One cannot obtain a lower weight lock while holding a lock with higher + * weight. The other way around is possible.
+ *
+ *

+ * For example: + *
+ * {@literal ->} acquire volume lock (will work)
+ * {@literal +->} acquire bucket lock (will work)
+ * {@literal +-->} acquire s3 bucket lock (will throw Exception)
+ *

+ *
+ */ + +public class OzoneManagerLock { + + private static final Logger LOG = + LoggerFactory.getLogger(OzoneManagerLock.class); + + private final LockManager manager; + private final ThreadLocal lockSet = ThreadLocal.withInitial( + () -> new Short((short)0)); + + + /** + * Creates new OzoneManagerLock instance. + * @param conf Configuration object + */ + public OzoneManagerLock(Configuration conf) { + manager = new LockManager<>(conf); + } + + /** + * Acquire lock on resource. + * + * For S3_Bucket, VOLUME, BUCKET type resource, same thread acquiring lock + * again is allowed. + * + * For USER, PREFIX, S3_SECRET type resource, same thread acquiring lock + * again is not allowed. + * + * Special Note for UserLock: Single thread can acquire single user lock/ + * multi user lock. But not both at the same time. + * @param resourceName - Resource name on which user want to acquire lock. + * @param resource - Type of the resource. + */ + public void acquireLock(String resourceName, Resource resource) { + if (!resource.canLock(lockSet.get())) { + String errorMessage = getErrorMessage(resource); + LOG.error(errorMessage); + throw new RuntimeException(errorMessage); + } else { + manager.lock(resourceName); + lockSet.set(resource.setLock(lockSet.get())); + } + } + + private String getErrorMessage(Resource resource) { + return "Thread '" + Thread.currentThread().getName() + "' cannot " + + "acquire " + resource.name + " lock while holding " + + getCurrentLocks().toString() + " lock(s)."; + + } + + private List getCurrentLocks() { + List currentLocks = new ArrayList<>(); + int i=0; + short lockSetVal = lockSet.get(); + for (Resource value : Resource.values()) { + if ((lockSetVal & value.setMask) == value.setMask) { + currentLocks.add(value.name); + } + } + return currentLocks; + } + + /** + * Acquire lock on multiple users. + * @param oldUserResource + * @param newUserResource + */ + public void acquireMultiUserLock(String oldUserResource, + String newUserResource) { + Resource resource = Resource.USER; + if (!resource.canLock(lockSet.get())) { + String errorMessage = getErrorMessage(resource); + LOG.error(errorMessage); + throw new RuntimeException(errorMessage); + } else { + int compare = newUserResource.compareTo(oldUserResource); + if (compare < 0) { + manager.lock(newUserResource); + try { + manager.lock(oldUserResource); + } catch (RuntimeException ex) { + manager.unlock(newUserResource); + } + } else if (compare > 0) { + // If this locking fails, we throw exception to user. + manager.lock(oldUserResource); + try { + manager.lock(newUserResource); + } catch (RuntimeException ex) { + manager.unlock(oldUserResource); + } + } else { + // both users are equal. + manager.lock(oldUserResource); + } + lockSet.set(resource.setLock(lockSet.get())); + } + } + + /** + * Acquire lock on multiple users. + * @param oldUserResource + * @param newUserResource + */ + public void releaseMultiUserLock(String oldUserResource, + String newUserResource) { + Resource resource = Resource.USER; + int compare = newUserResource.compareTo(oldUserResource); + if (compare < 0) { + manager.unlock(newUserResource); + manager.unlock(oldUserResource); + } else if (compare > 0) { + manager.unlock(oldUserResource); + manager.unlock(newUserResource); + } else { + // both users are equal. + manager.unlock(oldUserResource); + } + lockSet.set(resource.clearLock(lockSet.get())); + } + + + public void releaseLock(String resourceName, Resource resource) { + + // TODO: Not checking release of higher order level lock happened while + // releasing lower order level lock, as for that we need counter for + // locks, as some locks support acquiring lock again. + manager.unlock(resourceName); + // clear lock + lockSet.set(resource.clearLock(lockSet.get())); + + } + + /** + * Resource defined in Ozone. + */ + public enum Resource { + // For S3 Bucket need to allow only for S3, that should be means only 1. + S3_BUCKET((byte) 0, "S3_BUCKET"), // = 1 + + // For volume need to allow both s3 bucket and volume. 01 + 10 = 11 (3) + VOLUME((byte) 1, "VOLUME"), // = 2 + + // For bucket we need to allow both s3 bucket, volume and bucket. Which + // is equal to 100 + 010 + 001 = 111 = 4 + 2 + 1 = 7 + BUCKET((byte) 2, "BUCKET"), // = 4 + + // For user we need to allow s3 bucket, volume, bucket and user lock. + // Which is 8 4 + 2 + 1 = 15 + USER((byte) 3, "USER"), // 15 + + S3_SECRET((byte) 4, "S3_SECRET"), // 31 + PREFIX((byte) 5, "PREFIX"); //63 + + // level of the resource + private byte lockLevel; + + // This will tell the value, till which we can allow locking. + private short mask; + + // This value will help during setLock, and also will tell whether we can + // re-acquire lock or not. + private short setMask; + + // Name of the resource. + private String name; + + Resource(byte pos, String name) { + this.lockLevel = pos; + for (int x = 0; x < lockLevel + 1; x++) { + this.mask += (short) Math.pow(2, x); + } + this.setMask = (short) Math.pow(2, lockLevel); + this.name = name; + } + + boolean canLock(short lockSetVal) { + + // For USER, S3_SECRET and PREFIX we shall not allow re-acquire locks at + // from single thread. 2nd condition is we have acquired one of these + // locks, but after that trying to acquire a lock with less than equal of + // lockLevel, we should disallow. + if (((USER.setMask & lockSetVal) == USER.setMask || + (S3_SECRET.setMask & lockSetVal) == S3_SECRET.setMask || + (PREFIX.setMask & lockSetVal) == PREFIX.setMask) + && setMask <= lockSetVal) { + return false; + } + + + // Our mask is the summation of bits of all previous possible locks. In + // other words it is the largest possible value for that bit position. + + // For example for Volume lock, bit position is 1, and mask is 3. Which + // is the largest value that can be represented with 2 bits is 3. + // Therefore if lockSet is larger than mask we have to return false i.e + // some other higher order lock has been acquired. + + return lockSetVal <= mask; + } + + /** + * Set Lock bits in lockSetVal. + * + * @param lockSetVal + * @return Updated value which has set lock bits. + */ + short setLock(short lockSetVal) { + System.out.println("acquire" + name + (short) (lockSetVal | setMask)); + return (short) (lockSetVal | setMask); + } + + /** + * Clear lock from lockSetVal. + * + * @param lockSetVal + * @return Updated value which has cleared lock bits. + */ + short clearLock(short lockSetVal) { + System.out.println("release" + name + (short) (lockSetVal & ~setMask)); + return (short) (lockSetVal & ~setMask); + } + + String getName() { + return name; + } + + short getMask() { + return mask; + } + } + +} + diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java new file mode 100644 index 0000000000000..85e3c702e6027 --- /dev/null +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java @@ -0,0 +1,55 @@ +package org.apache.hadoop.ozone.om.lock; + +import static org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX; +import static org.apache.hadoop.ozone.OzoneConsts.OM_S3_PREFIX; +import static org.apache.hadoop.ozone.OzoneConsts.OM_S3_SECRET; +import static org.apache.hadoop.ozone.OzoneConsts.OM_USER_PREFIX; +import static org.apache.hadoop.ozone.container.common.volume.RoundRobinVolumeChoosingPolicy.LOG; + +/** + * Utility class contains helper functions required for OM lock. + */ +public final class OzoneManagerLockUtil { + + + private OzoneManagerLockUtil() { + } + + /** + * Generate resource lock name for the given resource name. + * + * @param resource + * @param resourceName + */ + public static String generateResourceLockName( + OzoneManagerLock.Resource resource, String resourceName) { + + if (resource == OzoneManagerLock.Resource.S3_BUCKET) { + return OM_S3_PREFIX + resourceName; + } else if (resource == OzoneManagerLock.Resource.VOLUME) { + return OM_KEY_PREFIX + resourceName; + } else if (resource == OzoneManagerLock.Resource.USER) { + return OM_USER_PREFIX + resourceName; + } else if (resource == OzoneManagerLock.Resource.S3_SECRET) { + return OM_S3_SECRET + resourceName; + } else if (resource == OzoneManagerLock.Resource.PREFIX) { + return OM_S3_PREFIX + resourceName; + } else { + throw new IllegalArgumentException("Unidentified resource type is" + + " passed when Resource type is bucket"); + } + + } + + /** + * Generate bucket lock name. + * @param volumeName + * @param bucketName + */ + public static String generateBucketLockName(String volumeName, + String bucketName) { + return OM_KEY_PREFIX + volumeName + OM_KEY_PREFIX + bucketName; + + } + +} diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/package-info.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/package-info.java new file mode 100644 index 0000000000000..5feac5f6c0bea --- /dev/null +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/package-info.java @@ -0,0 +1,22 @@ +/** + * 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.ozone.om.lock; + +/** + * Classes related to ozone manager lock. + */ \ No newline at end of file diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java new file mode 100644 index 0000000000000..63ddedaceab84 --- /dev/null +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java @@ -0,0 +1,375 @@ +/** + * 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.ozone.om.lock; + +import java.util.ArrayList; +import java.util.LinkedList; +import java.util.List; +import java.util.Queue; +import java.util.Stack; +import java.util.UUID; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.junit.Assert; +import org.junit.Test; + +import org.apache.hadoop.hdds.conf.OzoneConfiguration; + +import static org.junit.Assert.fail; + +/** + * Class tests OzoneManagerLock. + */ +public class TestOzoneManagerLock { + + @Test + public void acquireResourceLock() { + String resourceName; + for (OzoneManagerLock.Resource resource : + OzoneManagerLock.Resource.values()) { + resourceName = generateResourceLockName(resource); + testResourceLock(resourceName, resource); + } + } + + private void testResourceLock(String resourceName, + OzoneManagerLock.Resource resource) { + OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); + lock.acquireLock(resourceName, resource); + lock.releaseLock(resourceName, resource); + Assert.assertTrue(true); + } + + @Test + public void reacquireResourceLock() { + String resourceName; + for (OzoneManagerLock.Resource resource : + OzoneManagerLock.Resource.values()) { + resourceName = generateResourceLockName(resource); + testResourceReacquireLock(resourceName, resource); + } + } + + private void testResourceReacquireLock(String resourceName, + OzoneManagerLock.Resource resource) { + OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); + + // Lock re-acquire not allowed by same thread. + if (resource == OzoneManagerLock.Resource.USER || + resource == OzoneManagerLock.Resource.S3_SECRET || + resource == OzoneManagerLock.Resource.PREFIX){ + lock.acquireLock(resourceName, resource); + try { + lock.acquireLock(resourceName, resource); + fail("reacquireResourceLock failed"); + } catch (RuntimeException ex) { + String message = "cannot acquire " + resource.getName() + " lock " + + "while holding [" + resource.getName() + "] lock(s)."; + Assert.assertTrue(ex.getMessage(), ex.getMessage().contains(message)); + } + lock.releaseLock(resourceName, resource); + Assert.assertTrue(true); + } else { + lock.acquireLock(resourceName, resource); + lock.acquireLock(resourceName, resource); + lock.releaseLock(resourceName, resource); + lock.releaseLock(resourceName, resource); + Assert.assertTrue(true); + } + } + + @Test + public void testLockingOrder() { + OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); + String resourceName; + + // What this test does is iterate all resources. For each resource + // acquire lock, and then in inner loop acquire all locks with higher + // lock level, finally release the locks. + for (OzoneManagerLock.Resource resource : + OzoneManagerLock.Resource.values()) { + Stack stack = new Stack<>(); + resourceName = generateResourceLockName(resource); + lock.acquireLock(resourceName, resource); + stack.push(new ResourceInfo(resourceName, resource)); + for (OzoneManagerLock.Resource higherResource : + OzoneManagerLock.Resource.values()) { + if (higherResource.getMask() > resource.getMask()) { + resourceName = generateResourceLockName(higherResource); + lock.acquireLock(resourceName, higherResource); + stack.push(new ResourceInfo(resourceName, higherResource)); + } + } + // Now release locks + while (!stack.empty()) { + ResourceInfo resourceInfo = stack.pop(); + lock.releaseLock(resourceInfo.getLockName(), + resourceInfo.getResource()); + } + } + Assert.assertTrue(true); + } + + @Test + public void testLockViolationsWithOneHigherLevelLock() { + OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); + for (OzoneManagerLock.Resource resource : + OzoneManagerLock.Resource.values()) { + for (OzoneManagerLock.Resource higherResource : + OzoneManagerLock.Resource.values()) { + if (higherResource.getMask() > resource.getMask()) { + String resourceName = generateResourceLockName(higherResource); + lock.acquireLock(resourceName, higherResource); + try { + lock.acquireLock(generateResourceLockName(resource), resource); + fail("testLockViolationsWithOneHigherLevelLock failed"); + } catch (RuntimeException ex) { + String message = "cannot acquire " + resource.getName() + " lock " + + "while holding [" + higherResource.getName() + "] lock(s)."; + Assert.assertTrue(ex.getMessage(), + ex.getMessage().contains(message)); + } + lock.releaseLock(resourceName, higherResource); + } + } + } + } + + @Test + public void testLockViolations() { + OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); + String resourceName; + + // What this test does is iterate all resources. For each resource + // acquire an higher level lock above the resource, and then take the the + // lock. This should fail. Like that it tries all error combinations. + for (OzoneManagerLock.Resource resource : + OzoneManagerLock.Resource.values()) { + Stack stack = new Stack<>(); + List currentLocks = new ArrayList<>(); + Queue queue = new LinkedList<>(); + for (OzoneManagerLock.Resource higherResource : + OzoneManagerLock.Resource.values()) { + if (higherResource.getMask() > resource.getMask()) { + resourceName = generateResourceLockName(higherResource); + lock.acquireLock(resourceName, higherResource); + stack.push(new ResourceInfo(resourceName, higherResource)); + currentLocks.add(higherResource.getName()); + queue.add(new ResourceInfo(resourceName, higherResource)); + // try to acquire lower level lock + try { + resourceName = generateResourceLockName(resource); + lock.acquireLock(resourceName, resource); + } catch (RuntimeException ex) { + String message = "cannot acquire " + resource.getName() + " lock " + + "while holding " + currentLocks.toString() + " lock(s)."; + Assert.assertTrue(ex.getMessage(), + ex.getMessage().contains(message)); + } + } + } + + // Now release locks + while (!stack.empty()) { + ResourceInfo resourceInfo = stack.pop(); + lock.releaseLock(resourceInfo.getLockName(), + resourceInfo.getResource()); + } + } + } + + @Test + public void releaseLockWithOutAcquiringLock() { + String userLock = + OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user3"); + OzoneManagerLock lock = + new OzoneManagerLock(new OzoneConfiguration()); + try { + lock.releaseLock(userLock, OzoneManagerLock.Resource.USER); + fail("releaseLockWithOutAcquiringLock failed"); + } catch (IllegalMonitorStateException ex) { + String message = "Releasing lock on resource $user3 without acquiring " + + "lock"; + Assert.assertTrue(ex.getMessage(), ex.getMessage().contains(message)); + } + } + + + private String generateResourceLockName(OzoneManagerLock.Resource resource) { + if (resource == OzoneManagerLock.Resource.BUCKET) { + return OzoneManagerLockUtil.generateBucketLockName( + UUID.randomUUID().toString(), UUID.randomUUID().toString()); + } else { + return OzoneManagerLockUtil.generateResourceLockName(resource, + UUID.randomUUID().toString()); + } + } + + + /** + * Class used to store locked resource info. + */ + public class ResourceInfo { + private String lockName; + private OzoneManagerLock.Resource resource; + + ResourceInfo(String resourceName, OzoneManagerLock.Resource resource) { + this.lockName = resourceName; + this.resource = resource; + } + + public String getLockName() { + return lockName; + } + + public OzoneManagerLock.Resource getResource() { + return resource; + } + } + + @Test + public void acquireMultiUserLock() { + OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); + String oldUserLock = OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user1"); + String newUserLock = OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user2"); + lock.acquireMultiUserLock(oldUserLock, newUserLock); + lock.releaseMultiUserLock(oldUserLock, newUserLock); + Assert.assertTrue(true); + } + + @Test + public void reAcquireMultiUserLock() { + OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); + String oldUserLock = OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user1"); + String newUserLock = OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user2"); + lock.acquireMultiUserLock(oldUserLock, newUserLock); + try { + lock.acquireMultiUserLock(oldUserLock, newUserLock); + fail("reAcquireMultiUserLock failed"); + } catch (RuntimeException ex) { + String message = "cannot acquire USER lock while holding [USER] lock(s)."; + Assert.assertTrue(ex.getMessage(), ex.getMessage().contains(message)); + } + lock.releaseMultiUserLock(oldUserLock, newUserLock); + } + + @Test + public void acquireMultiUserLockAfterUserLock() { + OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); + String oldUserLock = OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user1"); + String newUserLock = OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user2"); + String userLock = OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user3"); + lock.acquireLock(userLock, OzoneManagerLock.Resource.USER); + try { + lock.acquireMultiUserLock(oldUserLock, newUserLock); + fail("acquireMultiUserLockAfterUserLock failed"); + } catch (RuntimeException ex) { + String message = "cannot acquire USER lock while holding [USER] lock(s)."; + Assert.assertTrue(ex.getMessage(), ex.getMessage().contains(message)); + } + lock.releaseLock(userLock, OzoneManagerLock.Resource.USER); + } + + @Test + public void acquireUserLockAfterMultiUserLock() { + OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); + String oldUserLock = OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user1"); + String newUserLock = OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user2"); + String userLock = OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user3"); + lock.acquireMultiUserLock(oldUserLock, newUserLock); + try { + lock.acquireLock(userLock, OzoneManagerLock.Resource.USER); + fail("acquireUserLockAfterMultiUserLock failed"); + } catch (RuntimeException ex) { + String message = "cannot acquire USER lock while holding [USER] lock(s)."; + Assert.assertTrue(ex.getMessage(), ex.getMessage().contains(message)); + } + lock.releaseMultiUserLock(oldUserLock, newUserLock); + } + + @Test + public void testLockResourceParallel() throws Exception { + OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); + + for (OzoneManagerLock.Resource resource : + OzoneManagerLock.Resource.values()) { + final String resourceName = generateResourceLockName(resource); + lock.acquireLock(resourceName, resource); + + AtomicBoolean gotLock = new AtomicBoolean(false); + new Thread(() -> { + lock.acquireLock(resourceName, resource); + gotLock.set(true); + lock.releaseLock(resourceName, resource); + }).start(); + // Let's give some time for the new thread to run + Thread.sleep(100); + // Since the new thread is trying to get lock on same volume, + // it will wait. + Assert.assertFalse(gotLock.get()); + lock.releaseLock(resourceName, OzoneManagerLock.Resource.VOLUME); + // Since we have released the lock, the new thread should have the lock + // now. + // Let's give some time for the new thread to run + Thread.sleep(100); + Assert.assertTrue(gotLock.get()); + } + + } + + @Test + public void testMultiLockResourceParallel() throws Exception { + OzoneManagerLock lock = new OzoneManagerLock(new OzoneConfiguration()); + + String oldUserLock = OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user1"); + String newUserLock = OzoneManagerLockUtil.generateResourceLockName( + OzoneManagerLock.Resource.USER, "user2"); + + lock.acquireMultiUserLock(oldUserLock, newUserLock); + + AtomicBoolean gotLock = new AtomicBoolean(false); + new Thread(() -> { + lock.acquireMultiUserLock(newUserLock, oldUserLock); + gotLock.set(true); + lock.releaseMultiUserLock(newUserLock, oldUserLock); + }).start(); + // Let's give some time for the new thread to run + Thread.sleep(100); + // Since the new thread is trying to get lock on same volume, it will wait. + Assert.assertFalse(gotLock.get()); + lock.releaseMultiUserLock(oldUserLock, newUserLock); + // Since we have released the lock, the new thread should have the lock + // now. + // Let's give some time for the new thread to run + Thread.sleep(100); + Assert.assertTrue(gotLock.get()); + } +} diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/package-info.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/package-info.java new file mode 100644 index 0000000000000..149794a5c6719 --- /dev/null +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/package-info.java @@ -0,0 +1,21 @@ +/** + * 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.ozone.om.lock; +/** + * Unit tests of OzoneManager lock. + */ From 6ae21084bfc41af3a7980368db6c077a34f4ed0c Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Fri, 21 Jun 2019 17:13:49 -0700 Subject: [PATCH 02/10] add logs --- .../org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java index 5b71cb455cb98..eadd57d301850 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java @@ -260,7 +260,7 @@ boolean canLock(short lockSetVal) { * @return Updated value which has set lock bits. */ short setLock(short lockSetVal) { - System.out.println("acquire" + name + (short) (lockSetVal | setMask)); + LOG.debug("acquire" + name + (short) (lockSetVal | setMask)); return (short) (lockSetVal | setMask); } @@ -271,7 +271,7 @@ short setLock(short lockSetVal) { * @return Updated value which has cleared lock bits. */ short clearLock(short lockSetVal) { - System.out.println("release" + name + (short) (lockSetVal & ~setMask)); + LOG.debug("release" + name + (short) (lockSetVal & ~setMask)); return (short) (lockSetVal & ~setMask); } From 9e3d823b28650c7d050a26732c57bdb373024a01 Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Fri, 21 Jun 2019 17:25:44 -0700 Subject: [PATCH 03/10] fix exception message --- .../apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java index 85e3c702e6027..ccd341e169500 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java @@ -4,7 +4,6 @@ import static org.apache.hadoop.ozone.OzoneConsts.OM_S3_PREFIX; import static org.apache.hadoop.ozone.OzoneConsts.OM_S3_SECRET; import static org.apache.hadoop.ozone.OzoneConsts.OM_USER_PREFIX; -import static org.apache.hadoop.ozone.container.common.volume.RoundRobinVolumeChoosingPolicy.LOG; /** * Utility class contains helper functions required for OM lock. @@ -35,8 +34,10 @@ public static String generateResourceLockName( } else if (resource == OzoneManagerLock.Resource.PREFIX) { return OM_S3_PREFIX + resourceName; } else { - throw new IllegalArgumentException("Unidentified resource type is" + - " passed when Resource type is bucket"); + // This is for developers who mistakenly call this method with resource + // bucket type, as for bucket type we need bucket and volumeName. + throw new IllegalArgumentException("Bucket resource type is passed, " + + "to get BucketResourceLockName, use generateBucketLockName method"); } } From daef5cd1c8e82eb63c659c076887a6786957a95d Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Fri, 21 Jun 2019 17:28:12 -0700 Subject: [PATCH 04/10] add license --- .../hadoop/ozone/om/lock/OzoneManagerLock.java | 18 ++++++++++++++++++ .../ozone/om/lock/OzoneManagerLockUtil.java | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java index eadd57d301850..381d21f426e52 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java @@ -1,3 +1,21 @@ +/** + * 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.ozone.om.lock; diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java index ccd341e169500..27f28f0e06c97 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLockUtil.java @@ -1,3 +1,21 @@ +/** + * 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.ozone.om.lock; import static org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX; From 2595084b6be813dbfd62e3030fb45b8dc2777734 Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Fri, 21 Jun 2019 17:30:02 -0700 Subject: [PATCH 05/10] fix findbug --- .../java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java index 381d21f426e52..085277eceda4d 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java @@ -77,7 +77,7 @@ public class OzoneManagerLock { private final LockManager manager; private final ThreadLocal lockSet = ThreadLocal.withInitial( - () -> new Short((short)0)); + () -> Short.valueOf((short)0)); /** From 7bff81097a7c964e02dfc8a1c0abe4ec658c7fde Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Mon, 24 Jun 2019 09:46:05 -0700 Subject: [PATCH 06/10] throw exception when acquiring 2nd lock failed. --- .../hadoop/ozone/om/lock/OzoneManagerLock.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java index 085277eceda4d..fa362fd41694e 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java @@ -150,16 +150,22 @@ public void acquireMultiUserLock(String oldUserResource, manager.lock(newUserResource); try { manager.lock(oldUserResource); - } catch (RuntimeException ex) { - manager.unlock(newUserResource); + } catch (Exception ex) { + // We got an exception acquiring 2nd user lock. Release already + // acquired user lock, and throw exception to the user. + manager.unlock(oldUserResource); + throw ex; } } else if (compare > 0) { // If this locking fails, we throw exception to user. manager.lock(oldUserResource); try { manager.lock(newUserResource); - } catch (RuntimeException ex) { + } catch (Exception ex) { + // We got an exception acquiring 2nd user lock. Release already + // acquired user lock, and throw exception to the user. manager.unlock(oldUserResource); + throw ex; } } else { // both users are equal. From 3ed81da0440407f7f24dc38cdc051e08de51e210 Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Mon, 24 Jun 2019 14:38:54 -0700 Subject: [PATCH 07/10] fix review comments --- .../ozone/om/lock/OzoneManagerLock.java | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java index fa362fd41694e..dcec31bd206e0 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java @@ -125,8 +125,8 @@ private List getCurrentLocks() { int i=0; short lockSetVal = lockSet.get(); for (Resource value : Resource.values()) { - if ((lockSetVal & value.setMask) == value.setMask) { - currentLocks.add(value.name); + if (value.isLevelLocked(lockSetVal)) { + currentLocks.add(value.getName()); } } return currentLocks; @@ -145,6 +145,21 @@ public void acquireMultiUserLock(String oldUserResource, LOG.error(errorMessage); throw new RuntimeException(errorMessage); } else { + // When acquiring multiple user locks, the reason for doing lexical + // order comparision is to avoid deadlock scenario. + + // Example: 1st thread acquire lock(ozone, hdfs) + // 2nd thread acquire lock(hdfs, ozone). + // If we don't acquire user locks in an order, there can be a deadlock. + + // 1st thread acquired lock on ozone, waiting for lock on hdfs, 2nd + // thread acquired lock on hdfs, waiting for lock on ozone. + // To avoid this when we acquire lock on multiple users, we acquire + // locks in lexical order, which can help us to avoid dead locks. + // Now if first thread acquires lock on hdfs, 2nd thread wait for lock + // on hdfs, and first thread acquires lock on ozone. Once after first + // thread releases user locks, 2nd thread acquires them. + int compare = newUserResource.compareTo(oldUserResource); if (compare < 0) { manager.lock(newUserResource); @@ -245,9 +260,7 @@ public enum Resource { Resource(byte pos, String name) { this.lockLevel = pos; - for (int x = 0; x < lockLevel + 1; x++) { - this.mask += (short) Math.pow(2, x); - } + this.mask += (short) Math.pow(2, lockLevel + 1) - 1; this.setMask = (short) Math.pow(2, lockLevel); this.name = name; } @@ -299,6 +312,10 @@ short clearLock(short lockSetVal) { return (short) (lockSetVal & ~setMask); } + boolean isLevelLocked(short lockSetVal) { + return (lockSetVal & setMask) == setMask; + } + String getName() { return name; } From a5d2490ceec847b215c8e8233c812b2a7f61fe3b Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Mon, 24 Jun 2019 14:54:07 -0700 Subject: [PATCH 08/10] fix missed review comment --- .../ozone/om/lock/OzoneManagerLock.java | 52 +++++++++++-------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java index dcec31bd206e0..f4263e8cc4979 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java @@ -22,6 +22,7 @@ import java.util.ArrayList; import java.util.List; +import com.ctc.wstx.util.StringUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -161,35 +162,35 @@ public void acquireMultiUserLock(String oldUserResource, // thread releases user locks, 2nd thread acquires them. int compare = newUserResource.compareTo(oldUserResource); - if (compare < 0) { + String temp; + + // Order the user names in sorted order. Swap them. + if (compare > 0) { + temp = newUserResource; + newUserResource = oldUserResource; + oldUserResource = temp; + } + + if (compare == 0) { + // both users are equal. + manager.lock(oldUserResource); + } else { manager.lock(newUserResource); try { manager.lock(oldUserResource); } catch (Exception ex) { // We got an exception acquiring 2nd user lock. Release already // acquired user lock, and throw exception to the user. - manager.unlock(oldUserResource); - throw ex; - } - } else if (compare > 0) { - // If this locking fails, we throw exception to user. - manager.lock(oldUserResource); - try { - manager.lock(newUserResource); - } catch (Exception ex) { - // We got an exception acquiring 2nd user lock. Release already - // acquired user lock, and throw exception to the user. - manager.unlock(oldUserResource); + manager.unlock(newUserResource); throw ex; } - } else { - // both users are equal. - manager.lock(oldUserResource); } lockSet.set(resource.setLock(lockSet.get())); } } + + /** * Acquire lock on multiple users. * @param oldUserResource @@ -199,14 +200,21 @@ public void releaseMultiUserLock(String oldUserResource, String newUserResource) { Resource resource = Resource.USER; int compare = newUserResource.compareTo(oldUserResource); - if (compare < 0) { - manager.unlock(newUserResource); - manager.unlock(oldUserResource); - } else if (compare > 0) { + + String temp; + + // Order the user names in sorted order. Swap them. + if (compare > 0) { + temp = newUserResource; + newUserResource = oldUserResource; + oldUserResource = temp; + } + + if (compare == 0) { + // both users are equal. manager.unlock(oldUserResource); - manager.unlock(newUserResource); } else { - // both users are equal. + manager.unlock(newUserResource); manager.unlock(oldUserResource); } lockSet.set(resource.clearLock(lockSet.get())); From d75851d8a06dd85fad0abf90e65bc8bc119c3a85 Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Mon, 24 Jun 2019 15:13:26 -0700 Subject: [PATCH 09/10] fix checkstyle --- .../java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java index f4263e8cc4979..16a3e132af49e 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java @@ -22,7 +22,6 @@ import java.util.ArrayList; import java.util.List; -import com.ctc.wstx.util.StringUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; From 919dc4c0cee591234845b8e0d0f3b77cd76dd187 Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Mon, 24 Jun 2019 21:42:29 -0700 Subject: [PATCH 10/10] fix review comments --- .../ozone/om/lock/OzoneManagerLock.java | 63 ++++++++++--------- .../ozone/om/lock/TestOzoneManagerLock.java | 1 - 2 files changed, 35 insertions(+), 29 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java index 16a3e132af49e..7d62bc5af9eed 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java @@ -109,6 +109,8 @@ public void acquireLock(String resourceName, Resource resource) { throw new RuntimeException(errorMessage); } else { manager.lock(resourceName); + LOG.debug("Acquired {} lock on resource {}", resource.name, + resourceName); lockSet.set(resource.setLock(lockSet.get())); } } @@ -134,11 +136,10 @@ private List getCurrentLocks() { /** * Acquire lock on multiple users. - * @param oldUserResource - * @param newUserResource + * @param firstUser + * @param secondUser */ - public void acquireMultiUserLock(String oldUserResource, - String newUserResource) { + public void acquireMultiUserLock(String firstUser, String secondUser) { Resource resource = Resource.USER; if (!resource.canLock(lockSet.get())) { String errorMessage = getErrorMessage(resource); @@ -151,7 +152,6 @@ public void acquireMultiUserLock(String oldUserResource, // Example: 1st thread acquire lock(ozone, hdfs) // 2nd thread acquire lock(hdfs, ozone). // If we don't acquire user locks in an order, there can be a deadlock. - // 1st thread acquired lock on ozone, waiting for lock on hdfs, 2nd // thread acquired lock on hdfs, waiting for lock on ozone. // To avoid this when we acquire lock on multiple users, we acquire @@ -160,30 +160,32 @@ public void acquireMultiUserLock(String oldUserResource, // on hdfs, and first thread acquires lock on ozone. Once after first // thread releases user locks, 2nd thread acquires them. - int compare = newUserResource.compareTo(oldUserResource); + int compare = firstUser.compareTo(secondUser); String temp; // Order the user names in sorted order. Swap them. if (compare > 0) { - temp = newUserResource; - newUserResource = oldUserResource; - oldUserResource = temp; + temp = secondUser; + secondUser = firstUser; + firstUser = temp; } if (compare == 0) { // both users are equal. - manager.lock(oldUserResource); + manager.lock(firstUser); } else { - manager.lock(newUserResource); + manager.lock(firstUser); try { - manager.lock(oldUserResource); + manager.lock(secondUser); } catch (Exception ex) { // We got an exception acquiring 2nd user lock. Release already // acquired user lock, and throw exception to the user. - manager.unlock(newUserResource); + manager.unlock(firstUser); throw ex; } } + LOG.debug("Acquired {} lock on resource {} and {}", resource.name, + firstUser, secondUser); lockSet.set(resource.setLock(lockSet.get())); } } @@ -191,31 +193,32 @@ public void acquireMultiUserLock(String oldUserResource, /** - * Acquire lock on multiple users. - * @param oldUserResource - * @param newUserResource + * Release lock on multiple users. + * @param firstUser + * @param secondUser */ - public void releaseMultiUserLock(String oldUserResource, - String newUserResource) { + public void releaseMultiUserLock(String firstUser, String secondUser) { Resource resource = Resource.USER; - int compare = newUserResource.compareTo(oldUserResource); + int compare = firstUser.compareTo(secondUser); String temp; // Order the user names in sorted order. Swap them. if (compare > 0) { - temp = newUserResource; - newUserResource = oldUserResource; - oldUserResource = temp; + temp = secondUser; + secondUser = firstUser; + firstUser = temp; } if (compare == 0) { // both users are equal. - manager.unlock(oldUserResource); + manager.unlock(firstUser); } else { - manager.unlock(newUserResource); - manager.unlock(oldUserResource); + manager.unlock(firstUser); + manager.unlock(secondUser); } + LOG.debug("Release {} lock on resource {} and {}", resource.name, + firstUser, secondUser); lockSet.set(resource.clearLock(lockSet.get())); } @@ -227,6 +230,8 @@ public void releaseLock(String resourceName, Resource resource) { // locks, as some locks support acquiring lock again. manager.unlock(resourceName); // clear lock + LOG.debug("Release {}, lock on resource {}", resource.name, + resource.name, resourceName); lockSet.set(resource.clearLock(lockSet.get())); } @@ -267,7 +272,7 @@ public enum Resource { Resource(byte pos, String name) { this.lockLevel = pos; - this.mask += (short) Math.pow(2, lockLevel + 1) - 1; + this.mask = (short) (Math.pow(2, lockLevel + 1) - 1); this.setMask = (short) Math.pow(2, lockLevel); this.name = name; } @@ -304,7 +309,6 @@ boolean canLock(short lockSetVal) { * @return Updated value which has set lock bits. */ short setLock(short lockSetVal) { - LOG.debug("acquire" + name + (short) (lockSetVal | setMask)); return (short) (lockSetVal | setMask); } @@ -315,10 +319,13 @@ short setLock(short lockSetVal) { * @return Updated value which has cleared lock bits. */ short clearLock(short lockSetVal) { - LOG.debug("release" + name + (short) (lockSetVal & ~setMask)); return (short) (lockSetVal & ~setMask); } + /** + * Return true, if this level is locked, else false. + * @param lockSetVal + */ boolean isLevelLocked(short lockSetVal) { return (lockSetVal & setMask) == setMask; } diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java index 63ddedaceab84..0df6cf2281d29 100644 --- a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java @@ -37,7 +37,6 @@ * Class tests OzoneManagerLock. */ public class TestOzoneManagerLock { - @Test public void acquireResourceLock() { String resourceName;