[GitHub] [hadoop] nandakumar131 commented on a change in pull request #1564: HDDS-2223. Support ReadWrite lock in LockManager.

[GitBox <gi...@apache.org>]

nandakumar131 commented on a change in pull request #1564: HDDS-2223. Support ReadWrite lock in LockManager.
URL: https://github.com/apache/hadoop/pull/1564#discussion_r330724170
 
 

 ##########
 File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/lock/LockManager.java
 ##########
 @@ -25,42 +25,146 @@
 
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Consumer;
 
 /**
  * Manages the locks on a given resource. A new lock is created for each
  * and every unique resource. Uniqueness of resource depends on the
  * {@code equals} implementation of it.
  */
-public class LockManager<T> {
+public class LockManager<R> {
 
   private static final Logger LOG = LoggerFactory.getLogger(LockManager.class);
 
-  private final Map<T, ActiveLock> activeLocks = new ConcurrentHashMap<>();
+  private final Map<R, ActiveLock> activeLocks = new ConcurrentHashMap<>();
   private final GenericObjectPool<ActiveLock> lockPool =
       new GenericObjectPool<>(new PooledLockFactory());
 
   /**
-   * Creates new LockManager instance.
+   * Creates new LockManager instance with the given Configuration.
    *
    * @param conf Configuration object
    */
-  public LockManager(Configuration conf) {
-    int maxPoolSize = conf.getInt(HddsConfigKeys.HDDS_LOCK_MAX_CONCURRENCY,
+  public LockManager(final Configuration conf) {
+    final int maxPoolSize = conf.getInt(HddsConfigKeys.HDDS_LOCK_MAX_CONCURRENCY,
         HddsConfigKeys.HDDS_LOCK_MAX_CONCURRENCY_DEFAULT);
     lockPool.setMaxTotal(maxPoolSize);
   }
 
-
   /**
    * Acquires the lock on given resource.
    *
    * <p>If the lock is not available then the current thread becomes
    * disabled for thread scheduling purposes and lies dormant until the
    * lock has been acquired.
+   *
+   * @param resource on which the lock has to be acquired
+   * @deprecated Use {@link LockManager#writeLock} instead
+   */
+  public void lock(final R resource) {
+   writeLock(resource);
+  }
+
+  /**
+   * Releases the lock on given resource.
+   *
+   * @param resource for which the lock has to be released
+   * @deprecated Use {@link LockManager#writeUnlock} instead
+   */
+  public void unlock(final R resource) {
+   writeUnlock(resource);
+  }
+
+  /**
+   * Acquires the read lock on given resource.
+   *
+   * <p>Acquires the read lock on resource if the write lock is not held by
+   * another thread and returns immediately.
+   *
+   * <p>If the write lock on resource is held by another thread then
+   * the current thread becomes disabled for thread scheduling
+   * purposes and lies dormant until the read lock has been acquired.
+   *
+   * @param resource on which the read lock has to be acquired
+   */
+  public void readLock(final R resource) {
+    acquire(resource, ActiveLock::readLock);
+  }
+
+  /**
+   * Releases the read lock on given resource.
+   *
+   * @param resource for which the read lock has to be released
+   * @throws IllegalMonitorStateException if the current thread does not
+   *                                      hold this lock
+   */
+  public void readUnlock(final R resource) throws IllegalMonitorStateException {
+    release(resource, ActiveLock::readUnlock);
+  }
+
+  /**
+   * Acquires the write lock on given resource.
+   *
+   * <p>Acquires the write lock on resource if neither the read nor write lock
+   * are held by another thread and returns immediately.
+   *
+   * <p>If the current thread already holds the write lock then the
+   * hold count is incremented by one and the method returns
+   * immediately.
+   *
+   * <p>If the lock is held by another thread then the current
+   * thread becomes disabled for thread scheduling purposes and
+   * lies dormant until the write lock has been acquired.
+   *
+   * @param resource on which the lock has to be acquired
    */
-  public void lock(T resource) {
-    activeLocks.compute(resource, (k, v) -> {
-      ActiveLock lock;
+  public void writeLock(final R resource) {
+    acquire(resource, ActiveLock::writeLock);
+  }
+
+  /**
+   * Releases the write lock on given resource.
+   *
+   * @param resource for which the lock has to be released
+   * @throws IllegalMonitorStateException if the current thread does not
+   *                                      hold this lock
+   */
+  public void writeUnlock(final R resource) throws IllegalMonitorStateException {
+    release(resource, ActiveLock::writeUnlock);
+  }
+
+  /**
+   * Acquires the lock on given resource using the provided lock function.
+   *
+   * @param resource on which the lock has to be acquired
+   * @param lockFn function to acquire the lock
+   */
+  private void acquire(final R resource, final Consumer<ActiveLock> lockFn) {
+    lockFn.accept(getLockForLocking(resource));
 
 Review comment:
   While acquiring the lock if we don't increment the active count as part of getLock call (atomically) we will end up in inconsistent state.
   
   Let's say we got the lock and didn't increment the count, we try to acquire the lock and some other thread is holding the same lock. The current thread will wait, we still haven't incremented the active counter. 
   While the other thread which was holding the lock will release the lock, as part of releasing we check the active lock count to remove lock object from the map and return the lock object to pool. In this case the active count will be 0 and we will end up removing the lock object from map, but the same lock object is given out.
   Now we have a lock which is given out, but the lock object is removed from the map and returned to the pool.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org