Improve code style and organization

core/common/src/main/java/alluxio/collections/Pair.java

@@ -20,7 +20,7 @@
  * @param <T2> the second element of the Pair
  */
 @NotThreadSafe
-public final class Pair<T1, T2> {
+public class Pair<T1, T2> {
   private T1 mFirst;
   private T2 mSecond;
 

core/common/src/main/java/alluxio/exception/ExceptionMessage.java

@@ -100,16 +100,16 @@ public enum ExceptionMessage {
   HDFS_FILE_NOT_FOUND("File {0} with id {1} is not found"),
 
   // file system master
-  PATH_MUST_HAVE_VALID_PARENT("{0} does not have a valid parent"),
   FILEID_MUST_BE_FILE("File id {0} must be a file"),
-  RENAME_CANNOT_BE_ONTO_MOUNT_POINT("{0} is a mount point and cannot be renamed onto"),
+  NOT_MUTABLE_INODE_PATH("Not a MutableLockedInodePath: {0}"),
+  PATH_COMPONENTS_INVALID("parameter pathComponents is {0}"),
+  PATH_COMPONENTS_INVALID_START("Path starts with {0}"),
+  PATH_MUST_HAVE_VALID_PARENT("{0} does not have a valid parent"),
   RENAME_CANNOT_BE_ACROSS_MOUNTS("Renaming {0} to {1} is a cross mount operation"),
+  RENAME_CANNOT_BE_ONTO_MOUNT_POINT("{0} is a mount point and cannot be renamed onto"),
   RENAME_CANNOT_BE_TO_ROOT("Cannot rename a path to the root directory"),
   RENAME_CANNOT_BE_TO_SUBDIRECTORY("Cannot rename because {0} is a prefix of {1}"),
   ROOT_CANNOT_BE_RENAMED("The root directory cannot be renamed"),
-  NOT_MUTABLE_INODE_PATH("Not a MutableLockedInodePath: {0}"),
-  PATH_COMPONENTS_INVALID("parameter pathComponents is {0}"),
-  PATH_COMPONENTS_INVALID_START("Path starts with {0}"),
 
   // file system master ufs
   FAILED_UFS_CREATE("Failed to create {0} in the under file system"),

core/server/src/main/java/alluxio/master/file/PermissionChecker.java

@@ -188,7 +188,7 @@ private List<String> getGroups(String user) throws AccessControlException {
   /**
    * Checks whether the client user is the owner of the path.
    *
-   * @param inodePath to be checked on
+   * @param inodePath path to be checked on
    * @throws AccessControlException if permission checking fails
    * @throws InvalidPathException if the path is invalid
    */

core/server/src/main/java/alluxio/master/file/meta/Inode.java

@@ -271,33 +271,47 @@ public T setUserName(String userName) {
   protected abstract T getThis();
 
   /**
-   * Lock this inode for reading.
+   * Acquires the read lock for this inode.
    */
   public void lockRead() {
     mReadLock.lock();
   }
 
   /**
-   * Read unlock this inode.
+   * Releases the read lock for this inode.
    */
   public void unlockRead() {
     mReadLock.unlock();
   }
 
   /**
-   * Lock this inode for writing/updating.
+   * Acquires the write lock for this inode.
    */
   public void lockWrite() {
     mWriteLock.lock();
   }
 
   /**
-   * Write unlock this inode.
+   * Releases the write lock for this inode.
    */
   public void unlockWrite() {
     mWriteLock.unlock();
   }
 
+  /**
+   * @return returns true if the current thread holds a write lock on this inode, false otherwise
+   */
+  public boolean isWriteLocked() {
+    return mLock.isWriteLockedByCurrentThread();
+  }
+
+  /**
+   * @return returns true if the current thread holds a read lock on this inode, false otherwise
+   */
+  public boolean isReadLocked() {
+    return mLock.getReadHoldCount() > 0;
+  }
+
   @Override
   public int hashCode() {
     return ((Long) mId).hashCode();

core/server/src/main/java/alluxio/master/file/meta/InodeLockGroup.java

@@ -11,6 +11,8 @@
 
 package alluxio.master.file.meta;
 
+import com.sun.org.apache.regexp.internal.RE;
+
 import java.util.ArrayList;
 import java.util.List;
 
@@ -21,14 +23,12 @@
  */
 @ThreadSafe
 public final class InodeLockGroup implements AutoCloseable {
-  private final List<Inode<?>> mReadLockedInodes;
-  private final List<Inode<?>> mWriteLockedInodes;
   private final List<Inode<?>> mInodes;
+  private final List<InodeTree.LockMode> mLockModes;
 
   InodeLockGroup() {
-    mReadLockedInodes = new ArrayList<>();
-    mWriteLockedInodes = new ArrayList<>();
     mInodes = new ArrayList<>();
+    mLockModes = new ArrayList<>();
   }
 
   /**
@@ -38,8 +38,8 @@ public final class InodeLockGroup implements AutoCloseable {
    */
   public synchronized void lockRead(Inode<?> inode) {
     inode.lockRead();
-    mReadLockedInodes.add(inode);
     mInodes.add(inode);
+    mLockModes.add(InodeTree.LockMode.READ);
   }
 
   /**
@@ -50,17 +50,11 @@ public synchronized void unlockLast() {
       return;
     }
     Inode<?> inode = mInodes.remove(mInodes.size() - 1);
-    if (mReadLockedInodes.size() > 0 && mReadLockedInodes.get(mReadLockedInodes.size() - 1)
-        .equals(inode)) {
+    InodeTree.LockMode lockMode = mLockModes.remove(mLockModes.size() - 1);
+    if (lockMode == InodeTree.LockMode.READ) {
       inode.unlockRead();
-      mReadLockedInodes.remove(mReadLockedInodes.size() - 1);
-      return;
-    }
-    if (mWriteLockedInodes.size() > 0 && mWriteLockedInodes.get(mWriteLockedInodes.size() - 1)
-        .equals(inode)) {
+    } else {
       inode.unlockWrite();
-      mWriteLockedInodes.remove(mWriteLockedInodes.size() - 1);
-      return;
     }
   }
 
@@ -71,8 +65,8 @@ public synchronized void unlockLast() {
    */
   public synchronized void lockWrite(Inode<?> inode) {
     inode.lockWrite();
-    mWriteLockedInodes.add(inode);
     mInodes.add(inode);
+    mLockModes.add(InodeTree.LockMode.WRITE);
   }
 
   /**
@@ -84,14 +78,16 @@ public synchronized List<Inode<?>> getInodes() {
 
   @Override
   public synchronized void close() {
-    for (Inode<?> inode : mReadLockedInodes) {
-      inode.unlockRead();
-    }
-    for (Inode<?> inode : mWriteLockedInodes) {
-      inode.unlockWrite();
+    for (int i = mInodes.size() - 1; i >= 0; i--) {
+      Inode<?> inode = mInodes.get(i);
+      InodeTree.LockMode lockMode = mLockModes.get(i);
+      if (lockMode == InodeTree.LockMode.READ) {
+        inode.unlockRead();
+      } else {
+        inode.unlockWrite();
+      }
     }
-    mReadLockedInodes.clear();
-    mWriteLockedInodes.clear();
     mInodes.clear();
+    mLockModes.clear();
   }
 }

core/server/src/main/java/alluxio/master/file/meta/InodePathPair.java

@@ -11,38 +11,35 @@
 
 package alluxio.master.file.meta;
 
+import alluxio.collections.Pair;
+
 import javax.annotation.concurrent.ThreadSafe;
 
 /**
- * This class represents a pair of {@link LockedInodePath}s.
+ * This class represents a pair of {@link LockedInodePath}s. This is threadsafe, since the
+ * elements cannot set once the pair is constructed.
  */
 @ThreadSafe
-public final class InodePathPair implements AutoCloseable {
-  private final LockedInodePath mInodePath1;
-  private final LockedInodePath mInodePath2;
+public final class InodePathPair extends Pair<LockedInodePath, LockedInodePath>
+    implements AutoCloseable {
 
   InodePathPair(LockedInodePath inodePath1, LockedInodePath inodePath2) {
-    mInodePath1 = inodePath1;
-    mInodePath2 = inodePath2;
+    super(inodePath1, inodePath2);
   }
 
-  /**
-   * @return the first of two {@link LockedInodePath}
-   */
-  public synchronized LockedInodePath getFirst() {
-    return mInodePath1;
+  @Override
+  public void setFirst(LockedInodePath first) {
+    throw new UnsupportedOperationException();
   }
 
-  /**
-   * @return the second of two {@link LockedInodePath}
-   */
-  public synchronized LockedInodePath getSecond() {
-    return mInodePath2;
+  @Override
+  public void setSecond(LockedInodePath second) {
+    throw new UnsupportedOperationException();
   }
 
   @Override
   public synchronized void close() {
-    mInodePath1.close();
-    mInodePath2.close();
+    getFirst().close();
+    getSecond().close();
   }
 }

core/server/src/main/java/alluxio/master/file/meta/InodeTree.java

@@ -355,14 +355,24 @@ public LockedInodePath lockFullInodePath(long id, LockMode lockMode)
       computePathForInode(inode, builder);
       AlluxioURI uri = new AlluxioURI(builder.toString());
 
-      try (LockedInodePath inodePath = lockFullInodePath(uri, lockMode)) {
+
+      boolean valid = false;
+      LockedInodePath inodePath = null;
+      try {
+        inodePath = lockFullInodePath(uri, lockMode);
         if (inodePath.getInode().getId() == id) {
+          // Set to true, so the path is not unlocked before returning.
+          valid = true;
           return inodePath;
         }
         // The path does not end up at the target inode id. Repeat the traversal.
       } catch (InvalidPathException e) {
         // ignore and repeat the loop
         LOG.warn("Inode lookup id {} computed path {} mismatch id. Repeating.", id, uri);
+      } finally {
+        if (!valid && inodePath != null) {
+          inodePath.close();
+        }
       }
       count++;
       if (count > PATH_TRAVERSAL_RETRIES) {
@@ -389,6 +399,13 @@ public void ensureFullInodePath(LockedInodePath inodePath, LockMode lockMode)
     }
   }
 
+  /**
+   * Appends components of the path from a given inode.
+   *
+   * @param inode the inode to compute the path for
+   * @param builder a {@link StringBuilder} that is updated with the path components
+   * @throws FileDoesNotExistException if an inode in the path does not exist
+   */
   private void computePathForInode(Inode<?> inode, StringBuilder builder)
       throws FileDoesNotExistException {
     inode.lockRead();
@@ -423,6 +440,7 @@ private void computePathForInode(Inode<?> inode, StringBuilder builder)
    * @throws FileDoesNotExistException if the path does not exist
    */
   public AlluxioURI getPath(Inode<?> inode) throws FileDoesNotExistException {
+    Preconditions.checkState(inode.isWriteLocked() || inode.isReadLocked());
     StringBuilder builder = new StringBuilder();
     computePathForInode(inode, builder);
     return new AlluxioURI(builder.toString());
@@ -966,7 +984,10 @@ private TraversalResult traverseToInodeInternal(String[] pathComponents, List<In
         if (lockMode != LockMode.READ
             && getLockModeForComponent(i - 1, pathComponents.length, lockMode, lockHints)
             == LockMode.READ) {
-          // The lock mode is one of the WRITE modes, but the previous inode was READ locked.
+          // The target lock mode is one of the WRITE modes, but READ lock is already held. The
+          // READ lock cannot be upgraded atomically, it needs be released first before WRITE
+          // lock can be acquired. As a consequence, we need to recheck if the child we are
+          // looking for has not been created in the meantime.
           lockGroup.unlockLast();
           lockGroup.lockWrite(current);
           Inode recheckNext = ((InodeDirectory) current).getChild(pathComponents[i]);

core/server/src/main/java/alluxio/master/file/meta/MutableLockedInodePath.java

@@ -26,6 +26,7 @@
  */
 @ThreadSafe
 public final class MutableLockedInodePath extends LockedInodePath {
+  // TODO(gpang): restructure class hierarchy, rename class
   MutableLockedInodePath(AlluxioURI uri, List<Inode<?>> inodes, InodeLockGroup lockGroup)
       throws InvalidPathException {
     super(uri, inodes, lockGroup);

core/server/src/main/java/alluxio/master/file/meta/TempInodePathForDescendant.java

@@ -29,6 +29,7 @@
  * This allows methods to operate on descendants associated with an existing
  * {@link LockedInodePath}.
  */
+// TODO(gpang): can an iterator for a LockedInodePath handle functionality for this class?
 @ThreadSafe
 public final class TempInodePathForDescendant extends LockedInodePath {
   private AlluxioURI mDescendantUri;

core/server/src/test/java/alluxio/master/file/FileSystemMasterTest.java

@@ -25,6 +25,7 @@
 import alluxio.heartbeat.ManuallyScheduleHeartbeat;
 import alluxio.master.MasterContext;
 import alluxio.master.block.BlockMaster;
+import alluxio.master.file.meta.LockedInodePath;
 import alluxio.master.file.meta.PersistenceState;
 import alluxio.master.file.meta.TtlBucket;
 import alluxio.master.file.meta.TtlBucketPrivateAccess;

core/server/src/test/java/alluxio/master/file/meta/InodeTreeTest.java

@@ -451,15 +451,16 @@ public void getPathTest() throws Exception {
     }
 
     // test one level
-    InodeTree.CreatePathResult createResult = createPath(mTree, TEST_URI, sDirectoryOptions);
-    List<Inode<?>> created = createResult.getCreated();
-    Assert.assertEquals(new AlluxioURI("/test"), mTree.getPath(created.get(created.size() - 1)));
+    createPath(mTree, TEST_URI, sDirectoryOptions);
+    try (LockedInodePath inodePath = mTree.lockFullInodePath(TEST_URI, InodeTree.LockMode.READ)) {
+      Assert.assertEquals(new AlluxioURI("/test"), mTree.getPath(inodePath.getInode()));
+    }
 
     // test nesting
-    createResult = createPath(mTree, NESTED_URI, sNestedDirectoryOptions);
-    created = createResult.getCreated();
-    Assert.assertEquals(new AlluxioURI("/nested/test"),
-        mTree.getPath(created.get(created.size() - 1)));
+    createPath(mTree, NESTED_URI, sNestedDirectoryOptions);
+    try (LockedInodePath inodePath = mTree.lockFullInodePath(NESTED_URI, InodeTree.LockMode.READ)) {
+      Assert.assertEquals(new AlluxioURI("/nested/test"), mTree.getPath(inodePath.getInode()));
+    }
   }
 
   /**