HADOOP-1961. The -get option to dfs-shell works when a single filename

is specified.  (Raghu Angadi via dhruba)



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

CHANGES.txt

@@ -378,6 +378,9 @@ Release 0.14.2 - unreleased
     block and that source Datanode had failed.
     (Raghu Angadi via dhruba)
 
+    HADOOP-1961.  The -get option to dfs-shell works when a single filename
+    is specified.  (Raghu Angadi via dhruba)
+
 Release 0.14.1 - 2007-09-04
 
   BUG FIXES

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

@@ -146,7 +146,18 @@ public class FsShell extends Configured implements Tool {
       }
       cat(srcf);
     } else {
-      copyToLocal(fs, new Path(srcf), new File(dstf), copyCrc);
+      File dst = new File(dstf);      
+      Path src = new Path(srcf);
+      Path [] srcs = fs.globPaths(src);
+      boolean dstIsDir = dst.isDirectory(); 
+      if (srcs.length > 1 && !dstIsDir) {
+        throw new IOException("When copying multiple files, "
+                              + "destination should be a directory.");
+      }
+      for (Path srcPath : srcs) {
+        File dstFile = (dstIsDir ? new File(dst, srcPath.getName()) : dst);
+        copyToLocal(fs, srcPath, dstFile, copyCrc);
+      }
     }
   }
 
@@ -168,61 +179,42 @@ public class FsShell extends Configured implements Tool {
   private void copyToLocal(final FileSystem srcFS, final Path src,
                            final File dst, final boolean copyCrc)
     throws IOException {
-    if (srcFS.isDirectory(src)) { //src is a directory
-      dst.mkdir();
-      if (!dst.isDirectory()) {
-        throw new IOException("cannot create directory for local destination \""
-                              + dst + "\".");
+    /* Keep the structure similar to ChecksumFileSystem.copyToLocal(). 
+     * Ideal these two should just invoke FileUtil.copy() and not repeat
+     * recursion here. Of course, copy() should support two more options :
+     * copyCrc and useTmpFile (may be useTmpFile need not be an option).
+     */
+    
+    if (!srcFS.isDirectory(src)) {
+      if (dst.exists()) {
+        // match the error message in FileUtil.checkDest():
+        throw new IOException("Target " + dst + " already exists");
       }
-      for(Path p : srcFS.listPaths(src)) {
-        copyToLocal(srcFS, p,
-                srcFS.isDirectory(p)? new File(dst, p.getName()): dst, copyCrc);
+      
+      // use absolute name so that tmp file is always created under dest dir
+      File tmp = FileUtil.createLocalTempFile(dst.getAbsoluteFile(),
+                                              COPYTOLOCAL_PREFIX, true);
+      if (!FileUtil.copy(srcFS, src, tmp, false, srcFS.getConf())) {
+        throw new IOException("Failed to copy " + src + " to " + dst); 
+      }
+      
+      if (!tmp.renameTo(dst)) {
+        throw new IOException("Failed to rename tmp file " + tmp + 
+                              " to local destination \"" + dst + "\".");
       }
-    }
-    else {
-      Path [] srcs = srcFS.globPaths(src);
-      if (dst.isDirectory()) { //dst is a directory but src is not
-        for (Path p : srcs) {
-          copyToLocal(srcFS, p, new File(dst, p.getName()), copyCrc);
-        }
-      } else if (srcs.length == 1)
-      {
-        if (dst.exists()) {
-          throw new IOException("local destination \"" + dst
-                                + "\" already exists.");
-        }
-        if (!srcFS.exists(src)) {
-          throw new IOException("src \"" + src + "\" does not exist.");
-        }
-
-        File tmp = FileUtil.createLocalTempFile(dst, COPYTOLOCAL_PREFIX, true);
-        if (FileUtil.copy(srcFS, src, tmp, false, srcFS.getConf())) {
-          if (!tmp.renameTo(dst)) {
-          //try to reanme tmp to another file since tmp will be deleted on exit
-            File another = FileUtil.createLocalTempFile(dst, COPYTOLOCAL_PREFIX,
-                                                        false);
-            another.delete();
-            if (tmp.renameTo(another)) {
-              throw new IOException(
-                  "Failed to rename tmp file to local destination \"" + dst
-                  + "\".  Remote source file \"" + src + "\" is saved to \""
-                  + another + "\".");
-            } else {
-              throw new IOException("Failed to rename tmp file.");
-            }
-          }
-        }
 
-        if (copyCrc) {
-          ChecksumFileSystem csfs = (ChecksumFileSystem) srcFS;
-          File dstcs = FileSystem.getLocal(srcFS.getConf())
-            .pathToFile(csfs.getChecksumFile(new Path(dst.getCanonicalPath())));
-          copyToLocal(csfs.getRawFileSystem(), csfs.getChecksumFile(src),
-                      dstcs, false);
-        }
-      } else {
-        throw new IOException("When copying multiple files, "
-                              + "destination should be a directory.");
+      if (copyCrc) {
+        ChecksumFileSystem csfs = (ChecksumFileSystem) srcFS;
+        File dstcs = FileSystem.getLocal(srcFS.getConf())
+          .pathToFile(csfs.getChecksumFile(new Path(dst.getCanonicalPath())));
+        copyToLocal(csfs.getRawFileSystem(), csfs.getChecksumFile(src),
+                    dstcs, false);
+      } 
+    } else {
+      // once FileUtil.copy() supports tmp file, we don't need to mkdirs().
+      dst.mkdirs();
+      for(Path path : srcFS.listPaths(src)) {
+        copyToLocal(srcFS, path, new File(dst, path.getName()), copyCrc);
       }
     }
   }

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

@@ -195,18 +195,22 @@ public class TestDFSShell extends TestCase {
         //   + sub
         //      |- f3
         //      |- f4
+        //   ROOT2
+        //   |- f1
         Path root = mkdir(dfs, new Path("/test/copyToLocal"));
         Path sub = mkdir(dfs, new Path(root, "sub"));
+        Path root2 = mkdir(dfs, new Path("/test/copyToLocal2"));        
 
         writeFile(fs, new Path(root, "f1"));
         writeFile(fs, new Path(root, "f2"));
         writeFile(fs, new Path(sub, "f3"));
         writeFile(fs, new Path(sub, "f4"));
+        writeFile(fs, new Path(root2, "f1"));        
       }
 
       // Verify copying the tree
       {
-        String[] args = {"-copyToLocal", "/test/copyToLocal", TEST_ROOT_DIR};
+        String[] args = {"-copyToLocal", "/test/copyToLocal*", TEST_ROOT_DIR};
         try {
           assertEquals(0, shell.run(args));
         } catch (Exception e) {
@@ -214,13 +218,16 @@ public class TestDFSShell extends TestCase {
                              e.getLocalizedMessage());
         }
 
-        File f1 = new File(TEST_ROOT_DIR, "f1");
+        File localroot = new File(TEST_ROOT_DIR, "copyToLocal");
+        File localroot2 = new File(TEST_ROOT_DIR, "copyToLocal2");        
+        
+        File f1 = new File(localroot, "f1");
         assertTrue("Copying failed.", f1.isFile());
 
-        File f2 = new File(TEST_ROOT_DIR, "f2");
+        File f2 = new File(localroot, "f2");
         assertTrue("Copying failed.", f2.isFile());
 
-        File sub = new File(TEST_ROOT_DIR, "sub");
+        File sub = new File(localroot, "sub");
         assertTrue("Copying failed.", sub.isDirectory());
 
         File f3 = new File(sub, "f3");
@@ -228,11 +235,15 @@ public class TestDFSShell extends TestCase {
 
         File f4 = new File(sub, "f4");
         assertTrue("Copying failed.", f4.isFile());
+        
+        File f5 = new File(localroot2, "f1");
+        assertTrue("Copying failed.", f5.isFile());        
 
         f1.delete();
         f2.delete();
         f3.delete();
         f4.delete();
+        f5.delete();
         sub.delete();
       }
     } finally {
@@ -290,7 +301,7 @@ public class TestDFSShell extends TestCase {
 
       // Verify that we can get with and without crc
       {
-        File testFile = new File(TEST_ROOT_DIR, "myFile");
+        File testFile = new File(TEST_ROOT_DIR, "mkdirs/myFile");
         File checksumFile = new File(fileSys.getChecksumFile(
                                                              new Path(testFile.getAbsolutePath())).toString());
         testFile.delete();
@@ -313,7 +324,7 @@ public class TestDFSShell extends TestCase {
         testFile.delete();
       }
       {
-        File testFile = new File(TEST_ROOT_DIR, "myFile");
+        File testFile = new File(TEST_ROOT_DIR, "mkdirs/myFile");
         File checksumFile = new File(fileSys.getChecksumFile(
                                                              new Path(testFile.getAbsolutePath())).toString());
         testFile.delete();