diff --git a/CHANGES.txt b/CHANGES.txt index 54d04f9979..5fc613a7ad 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1128,6 +1128,10 @@ Release 0.21.0 - Unreleased 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 diff --git a/src/java/org/apache/hadoop/fs/Path.java b/src/java/org/apache/hadoop/fs/Path.java index fb28010271..98829a2616 100644 --- a/src/java/org/apache/hadoop/fs/Path.java +++ b/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); } } diff --git a/src/java/org/apache/hadoop/util/GenericOptionsParser.java b/src/java/org/apache/hadoop/util/GenericOptionsParser.java index 55ede028b5..dc13feddf9 100644 --- a/src/java/org/apache/hadoop/util/GenericOptionsParser.java +++ b/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 Configuration 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("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)); - } - } 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 = new ArrayList(); + 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 { diff --git a/src/test/core/org/apache/hadoop/fs/TestPath.java b/src/test/core/org/apache/hadoop/fs/TestPath.java index 4fa28bc77c..62042fc6f6 100644 --- a/src/test/core/org/apache/hadoop/fs/TestPath.java +++ b/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()); + } } diff --git a/src/test/core/org/apache/hadoop/util/TestGenericOptionsParser.java b/src/test/core/org/apache/hadoop/util/TestGenericOptionsParser.java new file mode 100644 index 0000000000..5b48db1a58 --- /dev/null +++ b/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(); + } + +}