HADOOP-6334. Fix GenericOptionsParser and Path to have a better URI support. Contributed by Amareshwari Sriramadasu

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.21@831413 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -1096,6 +1096,10 @@ Trunk (unreleased changes)
 
     HADOOP-6318. Upgrade to Avro 1.2.0.  (cutting)
 
+    HADOOP-6334.  Fix GenericOptionsParser to understand URI for -files,
+    -libjars and -archives options and fix Path to support URI with fragment.
+    (Amareshwari Sriramadasu via szetszwo)
+
 Release 0.20.2 - Unreleased
 
   NEW FEATURES

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

@@ -63,13 +63,13 @@ public class Path implements Comparable {
     if (!(parentPath.equals("/") || parentPath.equals("")))
       try {
         parentUri = new URI(parentUri.getScheme(), parentUri.getAuthority(),
-                            parentUri.getPath()+"/", null, null);
+                      parentUri.getPath()+"/", null, parentUri.getFragment());
       } catch (URISyntaxException e) {
         throw new IllegalArgumentException(e);
       }
     URI resolved = parentUri.resolve(child.uri);
     initialize(resolved.getScheme(), resolved.getAuthority(),
-               normalizePath(resolved.getPath()));
+               normalizePath(resolved.getPath()), resolved.getFragment());
   }
 
   private void checkPathArg( String path ) {
@@ -123,7 +123,7 @@ public class Path implements Comparable {
     // uri path is the rest of the string -- query & fragment not supported
     String path = pathString.substring(start, pathString.length());
 
-    initialize(scheme, authority, path);
+    initialize(scheme, authority, path, null);
   }
 
   /**
@@ -136,12 +136,13 @@ public class Path implements Comparable {
   /** Construct a Path from components. */
   public Path(String scheme, String authority, String path) {
     checkPathArg( path );
-    initialize(scheme, authority, path);
+    initialize(scheme, authority, path, null);
   }
 
-  private void initialize(String scheme, String authority, String path) {
+  private void initialize(String scheme, String authority, String path,
+      String fragment) {
     try {
-      this.uri = new URI(scheme, authority, normalizePath(path), null, null)
+      this.uri = new URI(scheme, authority, normalizePath(path), null, fragment)
         .normalize();
     } catch (URISyntaxException e) {
       throw new IllegalArgumentException(e);
@@ -253,6 +254,10 @@ public class Path implements Comparable {
         path = path.substring(1);                 // remove slash before drive
       buffer.append(path);
     }
+    if (uri.getFragment() != null) {
+      buffer.append("#");
+      buffer.append(uri.getFragment());
+    }
     return buffer.toString();
   }
 
@@ -309,6 +314,7 @@ public class Path implements Comparable {
       
     String scheme = pathUri.getScheme();
     String authority = pathUri.getAuthority();
+    String fragment = pathUri.getFragment();
 
     if (scheme != null &&
         (authority != null || defaultUri.getAuthority() == null))
@@ -324,7 +330,14 @@ public class Path implements Comparable {
         authority = "";
       }
     }
-
-    return new Path(scheme+":"+"//"+authority + pathUri.getPath());
+    
+    URI newUri = null;
+    try {
+      newUri = new URI(scheme, authority , 
+        normalizePath(pathUri.getPath()), null, fragment);
+    } catch (URISyntaxException e) {
+      throw new IllegalArgumentException(e);
+    }
+    return new Path(newUri);
   }
 }

src/java/org/apache/hadoop/util/GenericOptionsParser.java

@@ -21,8 +21,11 @@ import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.PrintStream;
 import java.net.URI;
+import java.net.URISyntaxException;
 import java.net.URL;
 import java.net.URLClassLoader;
+import java.util.ArrayList;
+import java.util.List;
 
 import org.apache.commons.cli.CommandLine;
 import org.apache.commons.cli.CommandLineParser;
@@ -111,16 +114,20 @@ public class GenericOptionsParser {
    * Create an options parser with the given options to parse the args.
    * @param opts the options
    * @param args the command line arguments
+   * @throws IOException 
    */
-  public GenericOptionsParser(Options opts, String[] args) {
+  public GenericOptionsParser(Options opts, String[] args) 
+      throws IOException {
     this(new Configuration(), new Options(), args);
   }
 
   /**
    * Create an options parser to parse the args.
    * @param args the command line arguments
+   * @throws IOException 
    */
-  public GenericOptionsParser(String[] args) {
+  public GenericOptionsParser(String[] args) 
+      throws IOException {
     this(new Configuration(), new Options(), args);
   }
   
@@ -133,8 +140,10 @@ public class GenericOptionsParser {
    * 
    * @param conf the <code>Configuration</code> to modify.
    * @param args command-line arguments.
+   * @throws IOException 
    */
-  public GenericOptionsParser(Configuration conf, String[] args) {
+  public GenericOptionsParser(Configuration conf, String[] args) 
+      throws IOException {
     this(conf, new Options(), args); 
   }
 
@@ -148,8 +157,10 @@ public class GenericOptionsParser {
    * @param conf the configuration to modify  
    * @param options options built by the caller 
    * @param args User-specified arguments
+   * @throws IOException 
    */
-  public GenericOptionsParser(Configuration conf, Options options, String[] args) {
+  public GenericOptionsParser(Configuration conf,
+      Options options, String[] args) throws IOException {
     parseGeneralOptions(options, conf, args);
     this.conf = conf;
   }
@@ -240,7 +251,7 @@ public class GenericOptionsParser {
    * @param line User-specified generic options
    */
   private void processGeneralOptions(Configuration conf,
-      CommandLine line) {
+      CommandLine line) throws IOException {
     if (line.hasOption("fs")) {
       FileSystem.setDefaultUri(conf, line.getOptionValue("fs"));
     }
@@ -254,29 +265,25 @@ public class GenericOptionsParser {
         conf.addResource(new Path(value));
       }
     }
-    try {
-      if (line.hasOption("libjars")) {
-        conf.set("tmpjars", 
-                 validateFiles(line.getOptionValue("libjars"), conf));
-        //setting libjars in client classpath
-        URL[] libjars = getLibJars(conf);
-        if(libjars!=null && libjars.length>0) {
-          conf.setClassLoader(new URLClassLoader(libjars, conf.getClassLoader()));
-          Thread.currentThread().setContextClassLoader(
-              new URLClassLoader(libjars, 
-                  Thread.currentThread().getContextClassLoader()));
-        }
-      }
-      if (line.hasOption("files")) {
-        conf.set("tmpfiles", 
-                 validateFiles(line.getOptionValue("files"), conf));
-      }
-      if (line.hasOption("archives")) {
-        conf.set("tmparchives", 
-                  validateFiles(line.getOptionValue("archives"), conf));
+    if (line.hasOption("libjars")) {
+      conf.set("tmpjars", 
+               validateFiles(line.getOptionValue("libjars"), conf));
+      //setting libjars in client classpath
+      URL[] libjars = getLibJars(conf);
+      if(libjars!=null && libjars.length>0) {
+        conf.setClassLoader(new URLClassLoader(libjars, conf.getClassLoader()));
+        Thread.currentThread().setContextClassLoader(
+            new URLClassLoader(libjars, 
+                Thread.currentThread().getContextClassLoader()));
       }
-    } catch (IOException ioe) {
-      System.err.println(StringUtils.stringifyException(ioe));
+    }
+    if (line.hasOption("files")) {
+      conf.set("tmpfiles", 
+               validateFiles(line.getOptionValue("files"), conf));
+    }
+    if (line.hasOption("archives")) {
+      conf.set("tmparchives", 
+                validateFiles(line.getOptionValue("archives"), conf));
     }
     if (line.hasOption('D')) {
       String[] property = line.getOptionValues('D');
@@ -302,12 +309,14 @@ public class GenericOptionsParser {
       return null;
     }
     String[] files = jars.split(",");
-    URL[] cp = new URL[files.length];
-    for (int i=0;i<cp.length;i++) {
-      Path tmp = new Path(files[i]);
-      cp[i] = FileSystem.getLocal(conf).pathToFile(tmp).toURI().toURL();
+    List<URL> cp = new ArrayList<URL>();
+    for (String file : files) {
+      Path tmp = new Path(file);
+      if (tmp.getFileSystem(conf).equals(FileSystem.getLocal(conf))) {
+        cp.add(FileSystem.getLocal(conf).pathToFile(tmp).toURI().toURL());
+      }
     }
-    return cp;
+    return cp.toArray(new URL[0]);
   }
 
   /**
@@ -320,7 +329,8 @@ public class GenericOptionsParser {
    * @param files
    * @return
    */
-  private String validateFiles(String files, Configuration conf) throws IOException  {
+  private String validateFiles(String files, Configuration conf) 
+      throws IOException  {
     if (files == null) 
       return null;
     String[] fileArr = files.split(",");
@@ -328,8 +338,13 @@ public class GenericOptionsParser {
     for (int i =0; i < fileArr.length; i++) {
       String tmp = fileArr[i];
       String finalPath;
-      Path path = new Path(tmp);
-      URI pathURI =  path.toUri();
+      URI pathURI;
+      try {
+        pathURI = new URI(tmp);
+      } catch (URISyntaxException e) {
+        throw new IllegalArgumentException(e);
+      }
+      Path path = new Path(pathURI);
       FileSystem localFs = FileSystem.getLocal(conf);
       if (pathURI.getScheme() == null) {
         //default to the local file system
@@ -349,9 +364,6 @@ public class GenericOptionsParser {
           throw new FileNotFoundException("File " + tmp + " does not exist.");
         }
         finalPath = path.makeQualified(fs).toString();
-        try {
-          fs.close();
-        } catch(IOException e){};
       }
       finalArr[i] = finalPath;
     }
@@ -367,7 +379,7 @@ public class GenericOptionsParser {
    * @return Command-specific arguments
    */
   private String[] parseGeneralOptions(Options opts, Configuration conf, 
-      String[] args) {
+      String[] args) throws IOException {
     opts = buildGeneralOptions(opts);
     CommandLineParser parser = new GnuParser();
     try {

src/test/core/org/apache/hadoop/fs/TestPath.java

@@ -18,7 +18,12 @@
 
 package org.apache.hadoop.fs;
 
-import java.util.*;
+import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+
+import org.apache.hadoop.conf.Configuration;
+
 import junit.framework.TestCase;
 
 public class TestPath extends TestCase {
@@ -28,6 +33,8 @@ public class TestPath extends TestCase {
     toStringTest("/foo/bar");
     toStringTest("foo");
     toStringTest("foo/bar");
+    toStringTest("/foo/bar#boo");
+    toStringTest("foo/bar#boo");
     boolean emptyException = false;
     try {
       toStringTest("");
@@ -43,6 +50,8 @@ public class TestPath extends TestCase {
       toStringTest("c:foo/bar");
       toStringTest("c:foo/bar");
       toStringTest("c:/foo/bar");
+      toStringTest("C:/foo/bar#boo");
+      toStringTest("C:foo/bar#boo");
     }
   }
 
@@ -148,5 +157,25 @@ public class TestPath extends TestCase {
     assertEquals("foo://bar/baz", new Path("foo://bar/","/baz").toString()); 
   }
 
+  public void testURI() throws URISyntaxException, IOException {
+    URI uri = new URI("file:///bar#baz");
+    Path path = new Path(uri);
+    assertTrue(uri.equals(new URI(path.toString())));
+    
+    FileSystem fs = path.getFileSystem(new Configuration());
+    assertTrue(uri.equals(new URI(fs.makeQualified(path).toString())));
+    
+    // uri without hash
+    URI uri2 = new URI("file:///bar/baz");
+    assertTrue(
+      uri2.equals(new URI(fs.makeQualified(new Path(uri2)).toString())));
+    assertEquals("foo://bar/baz#boo", new Path("foo://bar/", new Path(new URI(
+        "/baz#boo"))).toString());
+    assertEquals("foo://bar/baz/fud#boo", new Path(new Path(new URI(
+        "foo://bar/baz#bud")), new Path(new URI("fud#boo"))).toString());
+    // if the child uri is absolute path
+    assertEquals("foo://bar/fud#boo", new Path(new Path(new URI(
+        "foo://bar/baz#bud")), new Path(new URI("/fud#boo"))).toString());
+ }
 
 }

src/test/core/org/apache/hadoop/util/TestGenericOptionsParser.java

@@ -0,0 +1,80 @@
+/**
+ * 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.util;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.net.URI;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+
+import junit.framework.TestCase;
+
+public class TestGenericOptionsParser extends TestCase {
+  private static File testDir = 
+    new File(System.getProperty("test.build.data", "/tmp"), "generic");
+  
+  public void testFilesOption() throws Exception {
+    Configuration conf = new Configuration();
+    File tmpFile = new File(testDir, "tmpfile");
+    FileSystem localFs = FileSystem.getLocal(conf);
+    Path tmpPath = new Path(tmpFile.toString());
+    localFs.create(tmpPath);
+    String[] args = new String[2];
+    // pass a files option 
+    args[0] = "-files";
+    args[1] = tmpFile.toString();
+    new GenericOptionsParser(conf, args);
+    String files = conf.get("tmpfiles");
+    assertNotNull("files is null", files);
+    assertEquals("files option does not match",
+      localFs.makeQualified(tmpPath).toString(), files);
+    
+    // pass file as uri
+    Configuration conf1 = new Configuration();
+    URI tmpURI = new URI(tmpFile.toString() + "#link");
+    args[0] = "-files";
+    args[1] = tmpURI.toString();
+    new GenericOptionsParser(conf1, args);
+    files = conf1.get("tmpfiles");
+    assertNotNull("files is null", files);
+    assertEquals("files option does not match", 
+      localFs.makeQualified(new Path(tmpURI)).toString(), files);
+   
+    // pass a file that does not exist.
+    // GenericOptionParser should throw exception
+    Configuration conf2 = new Configuration();
+    args[0] = "-files";
+    args[1] = "file:///xyz.txt";
+    Throwable th = null;
+    try {
+      new GenericOptionsParser(conf2, args);
+    } catch (Exception e) {
+      th = e;
+    }
+    assertNotNull("throwable is null", th);
+    assertTrue("FileNotFoundException is not thrown",
+      th instanceof FileNotFoundException);
+    files = conf2.get("tmpfiles");
+    assertNull("files is not null", files);
+    testDir.delete();
+  }
+
+}