From 1611b51a9780d18e76c0e9f9777bfae6ee70df12 Mon Sep 17 00:00:00 2001 From: Todd Lipcon Date: Thu, 28 Mar 2013 20:58:45 +0000 Subject: [PATCH] HADOOP-9150. Avoid unnecessary DNS resolution attempts for logical URIs. Contributed by Todd Lipcon. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1462303 13f79535-47bb-0310-9956-ffa450edef68 --- .../hadoop-common/CHANGES.txt | 3 + .../java/org/apache/hadoop/fs/FileSystem.java | 43 ++++++++++- .../apache/hadoop/fs/FilterFileSystem.java | 14 ++-- .../fs/TestFileSystemCanonicalization.java | 6 ++ .../apache/hadoop/test/GenericTestUtils.java | 19 ++++- .../hadoop/hdfs/DistributedFileSystem.java | 12 +++ .../apache/hadoop/hdfs/HftpFileSystem.java | 5 ++ .../hadoop/hdfs/web/WebHdfsFileSystem.java | 5 ++ .../hadoop/hdfs/TestDFSClientFailover.java | 77 +++++++++++++++++++ 9 files changed, 172 insertions(+), 12 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index 309ea50241a..6d8e2f93617 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -538,6 +538,9 @@ Release 2.0.5-beta - UNRELEASED OPTIMIZATIONS + HADOOP-9150. Avoid unnecessary DNS resolution attempts for logical URIs + (todd) + BUG FIXES HADOOP-9294. GetGroupsTestBase fails on Windows. (Chris Nauroth via suresh) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java index a26d3570586..ca80e91abc3 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java +++ b/hadoop-common-project/hadoop-common/src/main/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.security.PrivilegedExceptionAction; import java.util.ArrayList; import java.util.Arrays; @@ -211,12 +212,46 @@ public abstract class FileSystem extends Configured implements Closeable { public abstract URI getUri(); /** - * Resolve the uri's hostname and add the default port if not in the uri + * Return a canonicalized form of this FileSystem's URI. + * + * The default implementation simply calls {@link #canonicalizeUri(URI)} + * on the filesystem's own URI, so subclasses typically only need to + * implement that method. + * + * @see #canonicalizeUri(URI) + */ + protected URI getCanonicalUri() { + return canonicalizeUri(getUri()); + } + + /** + * Canonicalize the given URI. + * + * This is filesystem-dependent, but may for example consist of + * canonicalizing the hostname using DNS and adding the default + * port if not specified. + * + * The default implementation simply fills in the default port if + * not specified and if the filesystem has a default port. + * * @return URI * @see NetUtils#getCanonicalUri(URI, int) */ - protected URI getCanonicalUri() { - return NetUtils.getCanonicalUri(getUri(), getDefaultPort()); + protected URI canonicalizeUri(URI uri) { + if (uri.getPort() == -1 && getDefaultPort() > 0) { + // reconstruct the uri with the default port set + try { + uri = new URI(uri.getScheme(), uri.getUserInfo(), + uri.getHost(), getDefaultPort(), + uri.getPath(), uri.getQuery(), uri.getFragment()); + } catch (URISyntaxException e) { + // Should never happen! + throw new AssertionError("Valid URI became unparseable: " + + uri); + } + } + + return uri; } /** @@ -581,7 +616,7 @@ public abstract class FileSystem extends Configured implements Closeable { } if (uri != null) { // canonicalize uri before comparing with this fs - uri = NetUtils.getCanonicalUri(uri, getDefaultPort()); + uri = canonicalizeUri(uri); thatAuthority = uri.getAuthority(); if (thisAuthority == thatAuthority || // authorities match (thisAuthority != null && diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FilterFileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FilterFileSystem.java index a2b34f056ba..9fc8bb2d974 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FilterFileSystem.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FilterFileSystem.java @@ -95,16 +95,18 @@ public class FilterFileSystem extends FileSystem { public URI getUri() { return fs.getUri(); } - - /** - * Returns a qualified URI whose scheme and authority identify this - * FileSystem. - */ + + @Override protected URI getCanonicalUri() { return fs.getCanonicalUri(); } - + + @Override + protected URI canonicalizeUri(URI uri) { + return fs.canonicalizeUri(uri); + } + /** Make sure that a path specifies a FileSystem. */ @Override public Path makeQualified(Path path) { diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileSystemCanonicalization.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileSystemCanonicalization.java index ac9c05e89c8..2b8be39193a 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileSystemCanonicalization.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileSystemCanonicalization.java @@ -26,6 +26,7 @@ import java.net.URI; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.permission.FsPermission; +import org.apache.hadoop.net.NetUtils; import org.apache.hadoop.security.NetUtilsTestResolver; import org.apache.hadoop.util.Progressable; import org.junit.BeforeClass; @@ -312,6 +313,11 @@ public class TestFileSystemCanonicalization { return defaultPort; } + @Override + protected URI canonicalizeUri(URI uri) { + return NetUtils.getCanonicalUri(uri, getDefaultPort()); + } + @Override public FSDataInputStream open(Path f, int bufferSize) throws IOException { throw new IOException("not supposed to be here"); diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java index bfb52a8f928..5b08058d081 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java @@ -20,6 +20,7 @@ package org.apache.hadoop.test; import java.io.File; import java.io.IOException; import java.io.StringWriter; +import java.lang.reflect.InvocationTargetException; import java.util.Arrays; import java.util.Random; import java.util.Set; @@ -266,15 +267,29 @@ public abstract class GenericTestUtils { */ public static class DelegateAnswer implements Answer { private final Object delegate; + private final Log log; public DelegateAnswer(Object delegate) { + this(null, delegate); + } + + public DelegateAnswer(Log log, Object delegate) { + this.log = log; this.delegate = delegate; } @Override public Object answer(InvocationOnMock invocation) throws Throwable { - return invocation.getMethod().invoke( - delegate, invocation.getArguments()); + try { + if (log != null) { + log.info("Call to " + invocation + " on " + delegate, + new Exception("TRACE")); + } + return invocation.getMethod().invoke( + delegate, invocation.getArguments()); + } catch (InvocationTargetException ite) { + throw ite.getCause(); + } } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java index 3476f678708..e772859b8ee 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java @@ -62,6 +62,7 @@ import org.apache.hadoop.hdfs.security.token.block.InvalidBlockTokenException; import org.apache.hadoop.hdfs.security.token.delegation.DelegationTokenIdentifier; import org.apache.hadoop.hdfs.server.namenode.NameNode; import org.apache.hadoop.io.Text; +import org.apache.hadoop.net.NetUtils; import org.apache.hadoop.security.AccessControlException; import org.apache.hadoop.security.token.SecretManager.InvalidToken; import org.apache.hadoop.security.token.Token; @@ -893,6 +894,17 @@ public class DistributedFileSystem extends FileSystem { public String getCanonicalServiceName() { return dfs.getCanonicalServiceName(); } + + @Override + protected URI canonicalizeUri(URI uri) { + if (HAUtil.isLogicalUri(getConf(), uri)) { + // Don't try to DNS-resolve logical URIs, since the 'authority' + // portion isn't a proper hostname + return uri; + } else { + return NetUtils.getCanonicalUri(uri, getDefaultPort()); + } + } /** * Utility function that returns if the NameNode is in safemode or not. In HA diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HftpFileSystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HftpFileSystem.java index d97c62012e9..7d97c972532 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HftpFileSystem.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HftpFileSystem.java @@ -161,6 +161,11 @@ public class HftpFileSystem extends FileSystem // actual port in the uri return SecurityUtil.buildTokenService(nnSecureUri).toString(); } + + @Override + protected URI canonicalizeUri(URI uri) { + return NetUtils.getCanonicalUri(uri, getDefaultPort()); + } /** * Return the protocol scheme for the FileSystem. diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/web/WebHdfsFileSystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/web/WebHdfsFileSystem.java index 3a73d5c0e79..6f33827fa74 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/web/WebHdfsFileSystem.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/web/WebHdfsFileSystem.java @@ -238,6 +238,11 @@ public class WebHdfsFileSystem extends FileSystem public URI getUri() { return this.uri; } + + @Override + protected URI canonicalizeUri(URI uri) { + return NetUtils.getCanonicalUri(uri, getDefaultPort()); + } /** @return the home directory. */ public static String getHomeDirectoryString(final UserGroupInformation ugi) { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientFailover.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientFailover.java index 86bcae3a0a2..aacebce3595 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientFailover.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientFailover.java @@ -26,8 +26,11 @@ import java.io.IOException; import java.net.InetSocketAddress; import java.net.Socket; import java.net.SocketAddress; +import java.lang.reflect.Field; +import java.net.InetAddress; import java.net.URI; import java.net.URISyntaxException; +import java.util.List; import javax.net.SocketFactory; @@ -35,6 +38,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.CommonConfigurationKeysPublic; +import org.apache.hadoop.fs.FileContext; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hdfs.server.namenode.NameNode; @@ -48,10 +52,13 @@ import org.apache.hadoop.util.StringUtils; import org.hamcrest.BaseMatcher; import org.hamcrest.Description; import org.junit.After; +import org.junit.Assume; import org.junit.Before; import org.junit.Test; import org.mockito.Mockito; +import sun.net.spi.nameservice.NameService; + public class TestDFSClientFailover { private static final Log LOG = LogFactory.getLog(TestDFSClientFailover.class); @@ -201,4 +208,74 @@ public class TestDFSClientFailover { "Could not find any configured addresses for URI " + uri)); } } + + /** + * Spy on the Java DNS infrastructure. + * This likely only works on Sun-derived JDKs, but uses JUnit's + * Assume functionality so that any tests using it are skipped on + * incompatible JDKs. + */ + private NameService spyOnNameService() { + try { + Field f = InetAddress.class.getDeclaredField("nameServices"); + f.setAccessible(true); + Assume.assumeNotNull(f); + @SuppressWarnings("unchecked") + List nsList = (List) f.get(null); + + NameService ns = nsList.get(0); + Log log = LogFactory.getLog("NameServiceSpy"); + + ns = Mockito.mock(NameService.class, + new GenericTestUtils.DelegateAnswer(log, ns)); + nsList.set(0, ns); + return ns; + } catch (Throwable t) { + LOG.info("Unable to spy on DNS. Skipping test.", t); + // In case the JDK we're testing on doesn't work like Sun's, just + // skip the test. + Assume.assumeNoException(t); + throw new RuntimeException(t); + } + } + + /** + * Test that the client doesn't ever try to DNS-resolve the logical URI. + * Regression test for HADOOP-9150. + */ + @Test + public void testDoesntDnsResolveLogicalURI() throws Exception { + NameService spyNS = spyOnNameService(); + + FileSystem fs = HATestUtil.configureFailoverFs(cluster, conf); + String logicalHost = fs.getUri().getHost(); + Path qualifiedRoot = fs.makeQualified(new Path("/")); + + // Make a few calls against the filesystem. + fs.getCanonicalServiceName(); + fs.listStatus(qualifiedRoot); + + // Ensure that the logical hostname was never resolved. + Mockito.verify(spyNS, Mockito.never()).lookupAllHostAddr(Mockito.eq(logicalHost)); + } + + /** + * Same test as above, but for FileContext. + */ + @Test + public void testFileContextDoesntDnsResolveLogicalURI() throws Exception { + NameService spyNS = spyOnNameService(); + FileSystem fs = HATestUtil.configureFailoverFs(cluster, conf); + String logicalHost = fs.getUri().getHost(); + Configuration haClientConf = fs.getConf(); + + FileContext fc = FileContext.getFileContext(haClientConf); + Path root = new Path("/"); + fc.listStatus(root); + fc.listStatus(fc.makeQualified(root)); + fc.getDefaultFileSystem().getCanonicalServiceName(); + + // Ensure that the logical hostname was never resolved. + Mockito.verify(spyNS, Mockito.never()).lookupAllHostAddr(Mockito.eq(logicalHost)); + } }