From 74b46cf87e6b148e936b7cf3cbfbfd7aea1edf91 Mon Sep 17 00:00:00 2001 From: Thomas White Date: Tue, 15 Dec 2009 05:43:03 +0000 Subject: [PATCH] HADOOP-5901. FileSystem.fixName() has unexpected behaviour. Contributed by Aaron Kimball. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@890651 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES.txt | 3 + src/java/org/apache/hadoop/fs/FileSystem.java | 78 +++++++++++---- .../hadoop/fs/TestFileSystemCaching.java | 94 +++++++++++++++++++ 3 files changed, 157 insertions(+), 18 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 673340be3f8..c41313be532 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -94,6 +94,9 @@ Trunk (unreleased changes) HADOOP-6391. Classpath should not be part of command line arguments. (Cristian Ivascu via tomwhite) + HADOOP-5901. FileSystem.fixName() has unexpected behaviour. + (Aaron Kimball via tomwhite) + Release 0.21.0 - Unreleased INCOMPATIBLE CHANGES diff --git a/src/java/org/apache/hadoop/fs/FileSystem.java b/src/java/org/apache/hadoop/fs/FileSystem.java index 5121b44458e..73e22aaeaee 100644 --- a/src/java/org/apache/hadoop/fs/FileSystem.java +++ b/src/java/org/apache/hadoop/fs/FileSystem.java @@ -21,6 +21,7 @@ import java.io.Closeable; import java.io.FileNotFoundException; import java.io.IOException; import java.net.URI; +import java.net.URISyntaxException; import java.util.ArrayList; import java.util.Arrays; import java.util.EnumSet; @@ -108,7 +109,15 @@ public abstract class FileSystem extends Configured implements Closeable { * @return the uri of the default filesystem */ public static URI getDefaultUri(Configuration conf) { - return URI.create(fixName(conf.get(FS_DEFAULT_NAME_KEY, DEFAULT_FS))); + try { + String uri = conf.get(FS_DEFAULT_NAME_KEY, null); + checkName(uri); + return new URI(uri); + } catch (Exception e) { + // fs.default.name not set, or set to an invalid value. Create + // one based on a known-good URI + return URI.create(DEFAULT_FS); + } } /** Set the default filesystem URI in a configuration. @@ -122,9 +131,12 @@ public abstract class FileSystem extends Configured implements Closeable { /** Set the default filesystem URI in a configuration. * @param conf the configuration to alter * @param uri the new default filesystem uri + * @throws IOException if the URI is invalid. */ - public static void setDefaultUri(Configuration conf, String uri) { - setDefaultUri(conf, URI.create(fixName(uri))); + public static void setDefaultUri(Configuration conf, String uri) + throws IOException { + checkName(uri); + setDefaultUri(conf, URI.create(uri)); } /** Called after a new FileSystem instance is constructed. @@ -138,22 +150,52 @@ public abstract class FileSystem extends Configured implements Closeable { /** Returns a URI whose scheme and authority identify this FileSystem.*/ public abstract URI getUri(); - - /** Update old-format filesystem names, for back-compatibility. This should - * eventually be replaced with a checkName() method that throws an exception - * for old-format names. */ - private static String fixName(String name) { - // convert old-format name to new-format name - if (name.equals("local")) { // "local" is now "file:///". - LOG.warn("\"local\" is a deprecated filesystem name." - +" Use \"file:///\" instead."); - name = "file:///"; - } else if (name.indexOf('/')==-1) { // unqualified is "hdfs://" - LOG.warn("\""+name+"\" is a deprecated filesystem name." - +" Use \"hdfs://"+name+"/\" instead."); - name = "hdfs://"+name; + + /** Checks that a FileSystem name is given in an understandable format. + * The old "local" alias for "file:///" is unsupported, as are any + * URIs without a scheme component. + * @throws IOException if a name is in an unsupported format + */ + private static void checkName(String name) throws IOException { + if (null == name) { + throw new IOException("Null FS name provided to checkName()"); + } else if ("local".equals(name)) { + throw new IOException ("FileSystem 'local' is not supported; use 'file:///'"); + } else { + // Try parsing this into a URI + try { + URI uri = new URI(name); + + // No scheme; don't know how to parse this. + if (null == uri.getScheme()) { + throw new IOException("FileSystem name '" + name + + "' is provided in an unsupported format. (Try 'hdfs://" + + name + "' instead?)"); + } + + // This may have been a misparse. java.net.URI specifies that + // a URI is of the form: + // URI ::= [SCHEME-PART:]SCHEME-SPECIFIC-PART + // + // The scheme-specific-part may be parsed in numerous ways, but if + // it starts with a '/' character, that makes it a "hierarchical URI", + // subject to the following parsing: + // SCHEME-SPECIFIC-PART ::= "//" AUTHORITY-PART + // AUTHORITY-PART ::= [USER-INFO-PART] HOSTNAME [ ":" PORT ] + // + // In Hadoop, we require a host-based authority as well. + // java.net.URI parses left-to-right, so deprecated hostnames of the + // form 'foo:8020' will have 'foo' as their scheme and '8020' as their + // scheme-specific-part. We don't want this behavior. + if (null == uri.getAuthority()) { + throw new IOException("FileSystem name '" + name + + "' is provided in an unsupported format. (Try 'hdfs://" + + name + "' instead?)"); + } + } catch (URISyntaxException use) { + throw new IOException("FileSystem name cannot be understood as a URI", use); + } } - return name; } /** diff --git a/src/test/core/org/apache/hadoop/fs/TestFileSystemCaching.java b/src/test/core/org/apache/hadoop/fs/TestFileSystemCaching.java index 966ec6ae9a0..b134ff39e04 100644 --- a/src/test/core/org/apache/hadoop/fs/TestFileSystemCaching.java +++ b/src/test/core/org/apache/hadoop/fs/TestFileSystemCaching.java @@ -18,8 +18,12 @@ package org.apache.hadoop.fs; +import static junit.framework.Assert.assertEquals; import static junit.framework.Assert.assertSame; +import static junit.framework.Assert.assertNotNull; import static junit.framework.Assert.assertNotSame; +import static junit.framework.Assert.assertTrue; +import static junit.framework.Assert.fail; import java.net.URI; @@ -47,4 +51,94 @@ public class TestFileSystemCaching { assertNotSame(fs1, fs2); } + @Test + public void testGetLocal() throws Exception { + Configuration conf = new Configuration(); + conf.set(CommonConfigurationKeys.FS_DEFAULT_NAME_KEY, + CommonConfigurationKeys.FS_DEFAULT_NAME_DEFAULT); + FileSystem fs1 = FileSystem.get(conf); + assertTrue(fs1 instanceof LocalFileSystem); + + FileSystem fs2 = FileSystem.get(LocalFileSystem.NAME, conf); + assertTrue(fs2 instanceof LocalFileSystem); + } + + @Test + public void testGetDefaultFs() throws Exception { + Configuration conf = new Configuration(); + FileSystem fs = FileSystem.get(conf); + assertNotNull(fs); + } + + private void checkDefaultFsParam(Configuration conf, String uri) + throws Exception { + conf.set(CommonConfigurationKeys.FS_DEFAULT_NAME_KEY, uri); + assertNotNull(FileSystem.get(conf)); + } + + @Test + public void testInvalidDefaultFsParam() throws Exception { + Configuration conf = new Configuration(); + + // All of the following set the default filesystem config + // to an invalid URI. Subsequent requests for a FileSystem + // should return the internal default fs. + + checkDefaultFsParam(conf, "this-is-an-/invalid_uri"); + checkDefaultFsParam(conf, ""); + checkDefaultFsParam(conf, "/foo"); + checkDefaultFsParam(conf, "/foo/bar"); + checkDefaultFsParam(conf, "foo"); + checkDefaultFsParam(conf, "foo:8020"); + checkDefaultFsParam(conf, "foo/bar"); + checkDefaultFsParam(conf, "foo:8020/bar"); + checkDefaultFsParam(conf, "hdfs://"); + checkDefaultFsParam(conf, "local"); + } + + @Test + public void testGetInvalidFs() throws Exception { + Configuration conf = new Configuration(); + + // None of these are valid FileSystem URIs. The default FS + // should be returned in all cases. + + assertNotNull(FileSystem.get(URI.create("/foo"), conf)); + assertNotNull(FileSystem.get(URI.create("/foo/bar"), conf)); + assertNotNull(FileSystem.get(URI.create("foo"), conf)); + assertNotNull(FileSystem.get(URI.create("foo/bar"), conf)); + assertNotNull(FileSystem.get(URI.create("local"), conf)); + } + + private void checkBadUri(Configuration conf, String uri) { + try { + FileSystem.setDefaultUri(conf, uri); + fail("Expected invalid URI: " + uri); + } catch (Exception e) { + // Got expected exception; ok. + } + } + + @Test + public void testDefaultUri() throws Exception { + Configuration conf = new Configuration(); + URI uri = FileSystem.getDefaultUri(conf); + assertNotNull(uri.getScheme()); + + final URI exampleGoodUri = new URI("hdfs://foo.example.com:999"); + + FileSystem.setDefaultUri(conf, exampleGoodUri); + URI out = FileSystem.getDefaultUri(conf); + assertEquals(exampleGoodUri, out); + + checkBadUri(conf, "bla"); // no scheme + checkBadUri(conf, "local"); // deprecated syntax + checkBadUri(conf, "foo:8020"); // no scheme, deprecated syntax. + checkBadUri(conf, "hdfs://"); // not a valid uri; requires authority-part + checkBadUri(conf, ""); // not a uri. + + // Check that none of these actually changed the conf. + out = FileSystem.getDefaultUri(conf); + assertEquals(exampleGoodUri, out); + } }