HADOOP-2841. Unwrap FileNotFoundException and AccessControlException from RemoteException for DFSClient. Contributed by Konstantin Shvachko.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/core/trunk@644992 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -206,6 +206,9 @@ Trunk (unreleased changes)
     HADOOP-3099. Add an option to distcp to preserve user, group, and
     permission information. (Tsz Wo (Nicholas), SZE via cdouglas)
 
+    HADOOP-2841. Unwrap AccessControlException and FileNotFoundException
+    from RemoteException for DFSClient. (shv)
+
   OPTIMIZATIONS
 
     HADOOP-2790.  Fixed inefficient method hasSpeculativeTask by removing

src/java/org/apache/hadoop/dfs/DFSClient.java

@@ -22,6 +22,7 @@ import org.apache.hadoop.io.retry.RetryPolicies;
 import org.apache.hadoop.io.retry.RetryPolicy;
 import org.apache.hadoop.io.retry.RetryProxy;
 import org.apache.hadoop.fs.*;
+import org.apache.hadoop.fs.permission.AccessControlException;
 import org.apache.hadoop.fs.permission.FsPermission;
 import org.apache.hadoop.ipc.*;
 import org.apache.hadoop.net.NetUtils;
@@ -296,7 +297,17 @@ class DFSClient implements FSConstants {
     }
     return hints;
   }
-  
+
+  private LocatedBlocks callGetBlockLocations(String src, long start, 
+      long length) throws IOException {
+    try {
+      return namenode.getBlockLocations(src, start, length);
+    } catch(RemoteException re) {
+      throw re.unwrapRemoteException(AccessControlException.class,
+                                    FileNotFoundException.class);
+    }
+  }
+
   /**
    * Get block location info about file
    * 
@@ -311,7 +322,7 @@ class DFSClient implements FSConstants {
    */
   public BlockLocation[] getBlockLocations(String src, long start, 
     long length) throws IOException {
-    LocatedBlocks blocks = namenode.getBlockLocations(src, start, length);
+    LocatedBlocks blocks = callGetBlockLocations(src, start, length);
     if (blocks == null) {
       return new BlockLocation[0];
     }
@@ -483,7 +494,11 @@ class DFSClient implements FSConstants {
   public boolean setReplication(String src, 
                                 short replication
                                 ) throws IOException {
-    return namenode.setReplication(src, replication);
+    try {
+      return namenode.setReplication(src, replication);
+    } catch(RemoteException re) {
+      throw re.unwrapRemoteException(AccessControlException.class);
+    }
   }
 
   /**
@@ -492,7 +507,11 @@ class DFSClient implements FSConstants {
    */
   public boolean rename(String src, String dst) throws IOException {
     checkOpen();
-    return namenode.rename(src, dst);
+    try {
+      return namenode.rename(src, dst);
+    } catch(RemoteException re) {
+      throw re.unwrapRemoteException(AccessControlException.class);
+    }
   }
 
   /**
@@ -512,7 +531,11 @@ class DFSClient implements FSConstants {
    */
   public boolean delete(String src, boolean recursive) throws IOException {
     checkOpen();
-    return namenode.delete(src, recursive);
+    try {
+      return namenode.delete(src, recursive);
+    } catch(RemoteException re) {
+      throw re.unwrapRemoteException(AccessControlException.class);
+    }
   }
   
   /** Implemented using getFileInfo(src)
@@ -540,12 +563,56 @@ class DFSClient implements FSConstants {
    */
   public DFSFileInfo[] listPaths(String src) throws IOException {
     checkOpen();
-    return namenode.getListing(src);
+    try {
+      return namenode.getListing(src);
+    } catch(RemoteException re) {
+      throw re.unwrapRemoteException(AccessControlException.class);
+    }
   }
 
   public DFSFileInfo getFileInfo(String src) throws IOException {
     checkOpen();
-    return namenode.getFileInfo(src);
+    try {
+      return namenode.getFileInfo(src);
+    } catch(RemoteException re) {
+      throw re.unwrapRemoteException(AccessControlException.class,
+                                     FileNotFoundException.class);
+    }
+  }
+
+  /**
+   * Set permissions to a file or directory.
+   * @param src path name.
+   * @param permission
+   * @throws <code>FileNotFoundException</code> is file does not exist.
+   */
+  public void setPermission(String src, FsPermission permission
+                            ) throws IOException {
+    checkOpen();
+    try {
+      namenode.setPermission(src, permission);
+    } catch(RemoteException re) {
+      throw re.unwrapRemoteException(AccessControlException.class,
+                                     FileNotFoundException.class);
+    }
+  }
+
+  /**
+   * Set file or directory owner.
+   * @param src path name.
+   * @param username user id.
+   * @param groupname user group.
+   * @throws <code>FileNotFoundException</code> is file does not exist.
+   */
+  public void setOwner(String src, String username, String groupname
+                      ) throws IOException {
+    checkOpen();
+    try {
+      namenode.setOwner(src, username, groupname);
+    } catch(RemoteException re) {
+      throw re.unwrapRemoteException(AccessControlException.class,
+                                     FileNotFoundException.class);
+    }
   }
 
   public DiskStatus getDiskStatus() throws IOException {
@@ -643,11 +710,20 @@ class DFSClient implements FSConstants {
     }
     FsPermission masked = permission.applyUMask(FsPermission.getUMask(conf));
     LOG.debug(src + ": masked=" + masked);
-    return namenode.mkdirs(src, masked);
+    try {
+      return namenode.mkdirs(src, masked);
+    } catch(RemoteException re) {
+      throw re.unwrapRemoteException(AccessControlException.class);
+    }
   }
 
   ContentSummary getContentSummary(String src) throws IOException {
-    return namenode.getContentSummary(src);
+    try {
+      return namenode.getContentSummary(src);
+    } catch(RemoteException re) {
+      throw re.unwrapRemoteException(AccessControlException.class,
+                                     FileNotFoundException.class);
+    }
   }
 
   /**
@@ -1063,7 +1139,7 @@ class DFSClient implements FSConstants {
      * Grab the open-file info from namenode
      */
     synchronized void openInfo() throws IOException {
-      LocatedBlocks newInfo = namenode.getBlockLocations(src, 0, prefetchSize);
+      LocatedBlocks newInfo = callGetBlockLocations(src, 0, prefetchSize);
       if (newInfo == null) {
         throw new IOException("Cannot open filename " + src);
       }
@@ -1122,7 +1198,7 @@ class DFSClient implements FSConstants {
         targetBlockIdx = LocatedBlocks.getInsertIndex(targetBlockIdx);
         // fetch more blocks
         LocatedBlocks newBlocks;
-        newBlocks = namenode.getBlockLocations(src, offset, prefetchSize);
+        newBlocks = callGetBlockLocations(src, offset, prefetchSize);
         assert (newBlocks != null) : "Could not find target position " + offset;
         locatedBlocks.insertRange(targetBlockIdx, newBlocks.getLocatedBlocks());
       }
@@ -1161,7 +1237,7 @@ class DFSClient implements FSConstants {
           blk = locatedBlocks.get(blockIdx);
         if (blk == null || curOff < blk.getStartOffset()) {
           LocatedBlocks newBlocks;
-          newBlocks = namenode.getBlockLocations(src, curOff, remaining);
+          newBlocks = callGetBlockLocations(src, curOff, remaining);
           locatedBlocks.insertRange(blockIdx, newBlocks.getLocatedBlocks());
           continue;
         }
@@ -2114,8 +2190,12 @@ class DFSClient implements FSConstants {
       chunksPerPacket = Math.min(chunksPerBlock, 128);
       packetSize = chunkSize * chunksPerPacket;
 
-      namenode.create(
-          src, masked, clientName, overwrite, replication, blockSize);
+      try {
+        namenode.create(
+            src, masked, clientName, overwrite, replication, blockSize);
+      } catch(RemoteException re) {
+        throw re.unwrapRemoteException(AccessControlException.class);
+      }
       streamer = new DataStreamer();
       streamer.setDaemon(true);
       streamer.start();

src/java/org/apache/hadoop/dfs/DistributedFileSystem.java

@@ -23,7 +23,6 @@ import java.net.*;
 
 import org.apache.hadoop.fs.permission.FsPermission;
 import org.apache.hadoop.fs.*;
-import org.apache.hadoop.ipc.RemoteException;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.dfs.FSConstants.DatanodeReportType;
 import org.apache.hadoop.dfs.FSConstants.UpgradeAction;
@@ -127,20 +126,8 @@ public class DistributedFileSystem extends FileSystem {
   }
 
   public FSDataInputStream open(Path f, int bufferSize) throws IOException {
-    try {
-      return new DFSClient.DFSDataInputStream(
+    return new DFSClient.DFSDataInputStream(
           dfs.open(getPathName(f), bufferSize, verifyChecksum, statistics));
-    } catch(RemoteException e) {
-      if (IOException.class.getName().equals(e.getClassName()) &&
-          e.getMessage().startsWith(
-              "java.io.IOException: Cannot open filename")) {
-          // non-existent path
-          FileNotFoundException ne = new FileNotFoundException("File " + f + " does not exist.");
-          throw (FileNotFoundException) ne.initCause(e);
-      } else {
-        throw e;      // unexpected exception
-      }
-    }
   }
 
   public FSDataOutputStream create(Path f, FsPermission permission,
@@ -376,33 +363,13 @@ public class DistributedFileSystem extends FileSystem {
       return p.info;
     }
     
-    try {
-      DFSFileInfo p = dfs.getFileInfo(getPathName(f));
-      return p;
-    } catch (RemoteException e) {
-      if (IOException.class.getName().equals(e.getClassName()) &&
-          e.getMessage().startsWith(
-              "java.io.IOException: File does not exist: ")) {
-        // non-existent path
-        FileNotFoundException fe = new FileNotFoundException("File " + f + " does not exist.");
-        throw (FileNotFoundException) fe.initCause(e); 
-      } else {
-        throw e;      // unexpected exception
-      }
-    }
+    return dfs.getFileInfo(getPathName(f));
   }
 
   /** {@inheritDoc }*/
   public void setPermission(Path p, FsPermission permission
       ) throws IOException {
-    try {
-      dfs.namenode.setPermission(getPathName(p), permission);
-    } catch(RemoteException re) {
-      if(FileNotFoundException.class.getName().equals(re.getClassName())) {
-        throw new FileNotFoundException("File does not exist: " + p);
-      }
-      throw re;
-    }
+    dfs.setPermission(getPathName(p), permission);
   }
 
   /** {@inheritDoc }*/
@@ -411,13 +378,6 @@ public class DistributedFileSystem extends FileSystem {
     if (username == null && groupname == null) {
       throw new IOException("username == null && groupname == null");
     }
-    try {
-      dfs.namenode.setOwner(getPathName(p), username, groupname);
-    } catch(RemoteException re) {
-      if(FileNotFoundException.class.getName().equals(re.getClassName())) {
-        throw new FileNotFoundException("File does not exist: " + p);
-      }
-      throw re;
-    }
+    dfs.setOwner(getPathName(p), username, groupname);
   }
 }

src/java/org/apache/hadoop/dfs/FSDirectory.java

@@ -467,6 +467,7 @@ class FSDirectory implements FSConstants {
     }
     synchronized(rootDir) {
       INode targetNode = rootDir.getNode(src);
+      assert targetNode != null : "should be taken care in isDir() above";
       if (((INodeDirectory)targetNode).getChildren().size() != 0) {
         dirNotEmpty = false;
       }
@@ -569,12 +570,12 @@ class FSDirectory implements FSConstants {
    * @throws IOException if file does not exist
    * @return object containing information regarding the file
    */
-  DFSFileInfo getFileInfo(String src) throws IOException {
+  DFSFileInfo getFileInfo(String src) throws FileNotFoundException {
     String srcs = normalizePath(src);
     synchronized (rootDir) {
       INode targetNode = rootDir.getNode(srcs);
       if (targetNode == null) {
-        throw new IOException("File does not exist: " + srcs);
+        throw new FileNotFoundException("File does not exist: " + srcs);
       }
       else {
         return new DFSFileInfo(srcs, targetNode);
@@ -707,7 +708,7 @@ class FSDirectory implements FSConstants {
     synchronized (rootDir) {
       INode targetNode = rootDir.getNode(srcs);
       if (targetNode == null) {
-        throw new IOException(src + " does not exist");
+        throw new FileNotFoundException("File does not exist: " + srcs);
       }
       else {
         return targetNode.computeContentSummary();

src/java/org/apache/hadoop/fs/FsShell.java

@@ -40,6 +40,7 @@ import org.apache.hadoop.io.WritableComparable;
 import org.apache.hadoop.ipc.RPC;
 import org.apache.hadoop.ipc.RemoteException;
 import org.apache.hadoop.util.ReflectionUtils;
+import org.apache.hadoop.util.StringUtils;
 import org.apache.hadoop.util.Tool;
 import org.apache.hadoop.util.ToolRunner;
 import org.apache.hadoop.dfs.DistributedFileSystem;
@@ -982,7 +983,6 @@ public class FsShell extends Configured implements Tool {
     //
     if (argv.length > 3) {
       Path dst = new Path(dest);
-      FileSystem dstFs = dst.getFileSystem(getConf());
       if (!fs.isDirectory(dst)) {
         throw new IOException("When copying multiple files, " 
                               + "destination " + dest + " should be a directory.");
@@ -1091,7 +1091,6 @@ public class FsShell extends Configured implements Tool {
     CommandFormat c = new CommandFormat("tail", 1, 1, "f");
     String src = null;
     Path path = null;
-    short rep = 0;
 
     try {
       List<String> parameters = c.parse(cmd, pos);
@@ -1213,9 +1212,11 @@ public class FsShell extends Configured implements Tool {
             errors += runCmdHandler(handler, file, srcFs, recursive);
           }
         } catch (IOException e) {
-          System.err.println(handler.getName() + 
-                             ": could not get status for '" + path + "': " +
-                             e.getMessage().split("\n")[0]);        
+          String msg = (e.getMessage() != null ? e.getLocalizedMessage() :
+            (e.getCause().getMessage() != null ? 
+                e.getCause().getLocalizedMessage() : "null"));
+          System.err.println(handler.getName() + ": could not get status for '"
+                                        + path + "': " + msg.split("\n")[0]);        
         }
       }
     }
@@ -1350,10 +1351,12 @@ public class FsShell extends Configured implements Tool {
     String cat = "-cat <src>: \tFetch all files that match the file pattern <src> \n" +
       "\t\tand display their content on stdout.\n";
 
+    /*
     String text = "-text <path>: Attempt to decode contents if the first few bytes\n" +
       "\t\tmatch a magic number associated with a known format\n" +
       "\t\t(gzip, SequenceFile)\n";
-        
+    */
+
     String copyToLocal = COPYTOLOCAL_SHORT_USAGE
                          + ":  Identical to the -get command.\n";
 

src/java/org/apache/hadoop/fs/permission/AccessControlException.java

@@ -24,6 +24,12 @@ public class AccessControlException extends java.io.IOException {
   //Required by {@link java.io.Serializable}.
   private static final long serialVersionUID = 1L;
 
+  /**
+   * Default constructor is needed for unwrapping from 
+   * {@link org.apache.hadoop.ipc.RemoteException}.
+   */
+  public AccessControlException() {}
+
   /**
    * Constructs an {@link AccessControlException}
    * with the specified detail message.

src/java/org/apache/hadoop/ipc/RemoteException.java

@@ -31,4 +31,55 @@ public class RemoteException extends IOException {
   public String getClassName() {
     return className;
   }
+
+  /**
+   * If this remote exception wraps up one of the lookupTypes
+   * then return this exception.
+   * <p>
+   * Unwraps any IOException that has a default constructor.
+   * 
+   * @param lookupTypes the desired exception class.
+   * @return IOException, which is either the lookupClass exception or this.
+   */
+  public IOException unwrapRemoteException(Class... lookupTypes) {
+    if(lookupTypes == null)
+      return this;
+    for(Class lookupClass : lookupTypes) {
+      if(!IOException.class.isAssignableFrom(lookupClass))
+        continue;
+      if(lookupClass.getName().equals(getClassName())) {
+        try {
+          IOException ex = (IOException)lookupClass.newInstance();
+          ex.initCause(this);
+          return ex;
+        } catch(Exception e) {
+          // cannot instantiate lookupClass, just return this
+          return this;
+        }
+      } 
+    }
+    return this;
+  }
+
+  /**
+   * If this remote exception wraps an IOException that has a default
+   * contructor then instantiate and return the original exception.
+   * Otherwise return this.
+   * 
+   * @return IOException
+   */
+  public IOException unwrapRemoteException() {
+    IOException ex;
+    try {
+      Class realClass = Class.forName(getClassName());
+      if(!IOException.class.isAssignableFrom(realClass))
+        return this;
+      ex = (IOException)realClass.newInstance();
+      ex.initCause(this);
+      return ex;
+    } catch(Exception e) {
+      // cannot instantiate the original exception, just throw this
+    }
+    return this;
+  }
 }

src/test/org/apache/hadoop/dfs/TestDFSPermission.java

@@ -27,7 +27,6 @@ import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.dfs.MiniDFSCluster;
 import org.apache.hadoop.fs.*;
 import org.apache.hadoop.fs.permission.*;
-import org.apache.hadoop.ipc.RemoteException;
 import org.apache.hadoop.security.UnixUserGroupInformation;
 
 import junit.framework.AssertionFailedError;
@@ -37,8 +36,6 @@ import junit.framework.TestCase;
 public class TestDFSPermission extends TestCase {
   public static final Log LOG = LogFactory.getLog(TestDFSPermission.class);
   final private static Configuration conf = new Configuration();
-  final private static String PERMISSION_EXCEPTION_NAME = 
-    AccessControlException.class.getName();
   
   final private static String GROUP1_NAME = "group1";
   final private static String GROUP2_NAME = "group2";
@@ -236,12 +233,8 @@ public class TestDFSPermission extends TestCase {
       fs.setOwner(path, owner, group);
       checkOwnership(path, expectedOwner, expectedGroup);
       assertFalse(expectDeny);
-    } catch (RemoteException e) {
-      if (PERMISSION_EXCEPTION_NAME.equals(e.getClassName())) {
-        assertTrue(expectDeny);
-      } else {
-        throw e;
-      }
+    } catch(AccessControlException e) {
+      assertTrue(expectDeny);
     }
   }
 
@@ -490,12 +483,8 @@ public class TestDFSPermission extends TestCase {
         try {
           call();
           assertFalse(expectPermissionDeny());
-        } catch (RemoteException e) {
-          if (PERMISSION_EXCEPTION_NAME.equals(e.getClassName())) {
-            assertTrue(expectPermissionDeny());
-          } else {
-            throw e; // re-throw
-          }
+        } catch(AccessControlException e) {
+          assertTrue(expectPermissionDeny());
         }
       } catch (AssertionFailedError ae) {
         logPermissions();
@@ -966,9 +955,6 @@ public class TestDFSPermission extends TestCase {
   }
   
   private void checkNoPermissionDeny(IOException e) {
-    if (e instanceof RemoteException) {
-      assertFalse(PERMISSION_EXCEPTION_NAME.equals((
-          (RemoteException)e).getClassName()));
-    }
+    assertFalse(e instanceof AccessControlException);
   }
 }

src/test/org/apache/hadoop/security/TestPermission.java

@@ -27,7 +27,7 @@ import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.dfs.MiniDFSCluster;
 import org.apache.hadoop.fs.*;
 import org.apache.hadoop.fs.permission.*;
-import org.apache.hadoop.ipc.RemoteException;
+import org.apache.hadoop.util.StringUtils;
 import org.apache.log4j.Level;
 
 import junit.framework.TestCase;
@@ -47,9 +47,6 @@ public class TestPermission extends TestCase {
   final private static Path CHILD_FILE2 = new Path(ROOT_PATH, "file2");
 
   final private static int FILE_LEN = 100;
-  final private static String PERMISSION_EXCEPTION_NAME =
-    AccessControlException.class.getName();
-
   final private static String USER_NAME = "Who";
   final private static String GROUP1_NAME = "group1";
   final private static String GROUP2_NAME = "group2";
@@ -71,11 +68,13 @@ public class TestPermission extends TestCase {
     Configuration conf = new Configuration();
     conf.setBoolean("dfs.permissions", true);
     conf.setInt(FsPermission.UMASK_LABEL, 0);
-    MiniDFSCluster cluster = new MiniDFSCluster(conf, 3, true, null);
-    cluster.waitActive();
-    FileSystem fs = FileSystem.get(conf);
+    MiniDFSCluster cluster = null;
+    FileSystem fs = null;
 
     try {
+      cluster = new MiniDFSCluster(conf, 3, true, null);
+      cluster.waitActive();
+      fs = FileSystem.get(conf);
       FsPermission rootPerm = checkPermission(fs, "/", null);
       FsPermission inheritPerm = FsPermission.createImmutable(
           (short)(rootPerm.toShort() | 0300));
@@ -104,10 +103,17 @@ public class TestPermission extends TestCase {
           new FsPermission(permission));
       checkPermission(fs, "/c1", permission);
       checkPermission(fs, "/c1/c2.txt", permission);
-    }
-    finally {
-      try{fs.close();} catch(Exception e) {}
-      try{cluster.shutdown();} catch(Exception e) {}
+    } finally {
+      try {
+        if(fs != null) fs.close();
+      } catch(Exception e) {
+        LOG.error(StringUtils.stringifyException(e));
+      }
+      try {
+        if(cluster != null) cluster.shutdown();
+      } catch(Exception e) {
+        LOG.error(StringUtils.stringifyException(e));
+      }
     }
   }
 
@@ -148,11 +154,13 @@ public class TestPermission extends TestCase {
       // following read is legal
       byte dataIn[] = new byte[FILE_LEN];
       FSDataInputStream fin = fs.open(CHILD_FILE1);
-      fin.read(dataIn);
+      int bytesRead = fin.read(dataIn);
+      assertTrue(bytesRead == FILE_LEN);
       for(int i=0; i<FILE_LEN; i++) {
         assertEquals(data[i], dataIn[i]);
       }
       fs.close();
+      fs = null;
 
       // test illegal file/dir creation
       UnixUserGroupInformation userGroupInfo = new UnixUserGroupInformation(
@@ -173,10 +181,17 @@ public class TestPermission extends TestCase {
 
       // illegal file open
       assertTrue(!canOpen(fs, CHILD_FILE1));
-    }
-    finally {
-      try{fs.close();} catch(Exception e) {}
-      try{cluster.shutdown();} catch(Exception e) {}
+    } finally {
+      try {
+        if(fs != null) fs.close();
+      } catch(Exception e) {
+        LOG.error(StringUtils.stringifyException(e));
+      }
+      try {
+        if(cluster != null) cluster.shutdown();
+      } catch(Exception e) {
+        LOG.error(StringUtils.stringifyException(e));
+      }
     }
   }
 
@@ -184,8 +199,7 @@ public class TestPermission extends TestCase {
     try {
       fs.mkdirs(p);
       return true;
-    } catch(RemoteException e) {
-      assertEquals(PERMISSION_EXCEPTION_NAME, e.getClassName());
+    } catch(AccessControlException e) {
       return false;
     }
   }
@@ -194,8 +208,7 @@ public class TestPermission extends TestCase {
     try {
       fs.create(p);
       return true;
-    } catch(RemoteException e) {
-      assertEquals(PERMISSION_EXCEPTION_NAME, e.getClassName());
+    } catch(AccessControlException e) {
       return false;
     }
   }
@@ -204,8 +217,7 @@ public class TestPermission extends TestCase {
     try {
       fs.open(p);
       return true;
-    } catch(RemoteException e) {
-      assertEquals(PERMISSION_EXCEPTION_NAME, e.getClassName());
+    } catch(AccessControlException e) {
       return false;
     }
   }