From 32efc77799c792e15377e54eb42c6cef90b74ae0 Mon Sep 17 00:00:00 2001 From: Wei Yan Date: Wed, 11 Apr 2018 08:37:43 -0700 Subject: [PATCH] HDFS-13045. RBF: Improve error message returned from subcluster. Contributed by Inigo Goiri. (cherry picked from commit 0c93d43f3d624a4fd17b3b050443d9e7e20d4f0a) --- .../resolver/FederationNamespaceInfo.java | 5 ++ .../resolver/MountTableResolver.java | 4 +- .../federation/resolver/RemoteLocation.java | 35 ++++++---- .../router/RemoteLocationContext.java | 7 ++ .../federation/router/RouterRpcClient.java | 67 ++++++++++++++++++- .../federation/store/records/MountTable.java | 2 +- .../records/impl/pb/MountTablePBImpl.java | 2 +- .../hdfs/server/federation/MockResolver.java | 12 +++- .../federation/router/TestRouterAdmin.java | 4 +- .../federation/router/TestRouterRpc.java | 48 +++++++++++++ .../store/records/TestMountTable.java | 4 +- 11 files changed, 165 insertions(+), 25 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/FederationNamespaceInfo.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/FederationNamespaceInfo.java index edcd308d9d7..33edd30ec2e 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/FederationNamespaceInfo.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/FederationNamespaceInfo.java @@ -48,6 +48,11 @@ public class FederationNamespaceInfo extends RemoteLocationContext { return this.nameserviceId; } + @Override + public String getSrc() { + return null; + } + /** * The HDFS cluster id for this namespace. * diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MountTableResolver.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MountTableResolver.java index 97131384921..3f6efd6a615 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MountTableResolver.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MountTableResolver.java @@ -388,7 +388,7 @@ public class MountTableResolver } else { // Not found, use default location RemoteLocation remoteLocation = - new RemoteLocation(defaultNameService, path); + new RemoteLocation(defaultNameService, path, path); List locations = Collections.singletonList(remoteLocation); ret = new PathLocation(null, locations); @@ -519,7 +519,7 @@ public class MountTableResolver newPath += Path.SEPARATOR; } newPath += remainingPath; - RemoteLocation remoteLocation = new RemoteLocation(nsId, newPath); + RemoteLocation remoteLocation = new RemoteLocation(nsId, newPath, path); locations.add(remoteLocation); } DestinationOrder order = entry.getDestOrder(); diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/RemoteLocation.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/RemoteLocation.java index 6aa12ce2b14..77d050062e7 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/RemoteLocation.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/RemoteLocation.java @@ -20,8 +20,8 @@ package org.apache.hadoop.hdfs.server.federation.resolver; import org.apache.hadoop.hdfs.server.federation.router.RemoteLocationContext; /** - * A single in a remote namespace consisting of a nameservice ID - * and a HDFS path. + * A location in a remote namespace consisting of a nameservice ID and a HDFS + * path (destination). It also contains the federated location (source). */ public class RemoteLocation extends RemoteLocationContext { @@ -30,16 +30,19 @@ public class RemoteLocation extends RemoteLocationContext { /** Identifier of the namenode in the namespace for this location. */ private final String namenodeId; /** Path in the remote location. */ - private final String path; + private final String dstPath; + /** Original path in federation. */ + private final String srcPath; /** * Create a new remote location. * - * @param nsId - * @param pPath + * @param nsId Destination namespace. + * @param dPath Path in the destination namespace. + * @param sPath Path in the federated level. */ - public RemoteLocation(String nsId, String pPath) { - this(nsId, null, pPath); + public RemoteLocation(String nsId, String dPath, String sPath) { + this(nsId, null, dPath, sPath); } /** @@ -47,12 +50,15 @@ public class RemoteLocation extends RemoteLocationContext { * namespace. * * @param nsId Destination namespace. - * @param pPath Path in the destination namespace. + * @param nnId Destination namenode. + * @param dPath Path in the destination namespace. + * @param sPath Path in the federated level */ - public RemoteLocation(String nsId, String nnId, String pPath) { + public RemoteLocation(String nsId, String nnId, String dPath, String sPath) { this.nameserviceId = nsId; this.namenodeId = nnId; - this.path = pPath; + this.dstPath = dPath; + this.srcPath = sPath; } @Override @@ -66,11 +72,16 @@ public class RemoteLocation extends RemoteLocationContext { @Override public String getDest() { - return this.path; + return this.dstPath; + } + + @Override + public String getSrc() { + return this.srcPath; } @Override public String toString() { - return getNameserviceId() + "->" + this.path; + return getNameserviceId() + "->" + this.dstPath; } } \ No newline at end of file diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RemoteLocationContext.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RemoteLocationContext.java index a90c460a541..0959eaa34a8 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RemoteLocationContext.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RemoteLocationContext.java @@ -39,6 +39,13 @@ public abstract class RemoteLocationContext */ public abstract String getDest(); + /** + * Original source location. + * + * @return Source path. + */ + public abstract String getSrc(); + @Override public int hashCode() { return new HashCodeBuilder(17, 31) diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java index e2c9cb44d9a..6c657b2d6b0 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java @@ -18,6 +18,7 @@ package org.apache.hadoop.hdfs.server.federation.router; +import java.io.FileNotFoundException; import java.io.IOException; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; @@ -62,6 +63,7 @@ import org.apache.hadoop.security.UserGroupInformation; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.google.common.annotations.VisibleForTesting; import com.google.common.util.concurrent.ThreadFactoryBuilder; /** @@ -611,7 +613,7 @@ public class RouterRpcClient { UserGroupInformation ugi = RouterRpcServer.getRemoteUser(); List nns = getNamenodesForNameservice(nsId); - RemoteLocationContext loc = new RemoteLocation(nsId, "/"); + RemoteLocationContext loc = new RemoteLocation(nsId, "/", "/"); Class proto = method.getProtocol(); Method m = method.getMethod(); Object[] params = method.getParams(loc); @@ -727,8 +729,12 @@ public class RouterRpcClient { firstResult = result; } } catch (IOException ioe) { + // Localize the exception + + ioe = processException(ioe, loc); + // Record it and move on - lastThrownException = (IOException) ioe; + lastThrownException = ioe; if (firstThrownException == null) { firstThrownException = lastThrownException; } @@ -756,6 +762,63 @@ public class RouterRpcClient { return ret; } + /** + * Exception messages might contain local subcluster paths. This method + * generates a new exception with the proper message. + * @param ioe Original IOException. + * @param loc Location we are processing. + * @return Exception processed for federation. + */ + private IOException processException( + IOException ioe, RemoteLocationContext loc) { + + if (ioe instanceof RemoteException) { + RemoteException re = (RemoteException)ioe; + String newMsg = processExceptionMsg( + re.getMessage(), loc.getDest(), loc.getSrc()); + RemoteException newException = + new RemoteException(re.getClassName(), newMsg); + newException.setStackTrace(ioe.getStackTrace()); + return newException; + } + + if (ioe instanceof FileNotFoundException) { + String newMsg = processExceptionMsg( + ioe.getMessage(), loc.getDest(), loc.getSrc()); + FileNotFoundException newException = new FileNotFoundException(newMsg); + newException.setStackTrace(ioe.getStackTrace()); + return newException; + } + + return ioe; + } + + /** + * Process a subcluster message and make it federated. + * @param msg Original exception message. + * @param dst Path in federation. + * @param src Path in the subcluster. + * @return Message processed for federation. + */ + @VisibleForTesting + static String processExceptionMsg( + final String msg, final String dst, final String src) { + if (dst.equals(src) || !dst.startsWith("/") || !src.startsWith("/")) { + return msg; + } + + String newMsg = msg.replaceFirst(dst, src); + int minLen = Math.min(dst.length(), src.length()); + for (int i = 0; newMsg.equals(msg) && i < minLen; i++) { + // Check if we can replace sub folders + String dst1 = dst.substring(0, dst.length() - 1 - i); + String src1 = src.substring(0, src.length() - 1 - i); + newMsg = msg.replaceFirst(dst1, src1); + } + + return newMsg; + } + /** * Checks if a result matches the required result class. * diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/MountTable.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/MountTable.java index 60496efe479..005882ebdfa 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/MountTable.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/MountTable.java @@ -134,7 +134,7 @@ public abstract class MountTable extends BaseRecord { for (Entry entry : destinations.entrySet()) { String nsId = entry.getKey(); String path = normalizeFileSystemPath(entry.getValue()); - RemoteLocation location = new RemoteLocation(nsId, path); + RemoteLocation location = new RemoteLocation(nsId, path, src); locations.add(location); } diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/impl/pb/MountTablePBImpl.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/impl/pb/MountTablePBImpl.java index 48f93bc2abd..e62d0a83168 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/impl/pb/MountTablePBImpl.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/impl/pb/MountTablePBImpl.java @@ -102,7 +102,7 @@ public class MountTablePBImpl extends MountTable implements PBRecord { for (RemoteLocationProto dest : destList) { String nsId = dest.getNameserviceId(); String path = dest.getPath(); - RemoteLocation loc = new RemoteLocation(nsId, path); + RemoteLocation loc = new RemoteLocation(nsId, path, getSourcePath()); ret.add(loc); } return ret; diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/MockResolver.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/MockResolver.java index 151d73151eb..0ce0944854b 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/MockResolver.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/MockResolver.java @@ -80,7 +80,8 @@ public class MockResolver this.locations.put(mount, locationsList); } - final RemoteLocation remoteLocation = new RemoteLocation(nsId, location); + final RemoteLocation remoteLocation = + new RemoteLocation(nsId, location, mount); if (!locationsList.contains(remoteLocation)) { locationsList.add(remoteLocation); } @@ -270,10 +271,15 @@ public class MockResolver for (String key : keys) { if (path.startsWith(key)) { for (RemoteLocation location : this.locations.get(key)) { - String finalPath = location.getDest() + path.substring(key.length()); + String finalPath = location.getDest(); + String extraPath = path.substring(key.length()); + if (finalPath.endsWith("/") && extraPath.startsWith("/")) { + extraPath = extraPath.substring(1); + } + finalPath += extraPath; String nameservice = location.getNameserviceId(); RemoteLocation remoteLocation = - new RemoteLocation(nameservice, finalPath); + new RemoteLocation(nameservice, finalPath, path); remoteLocations.add(remoteLocation); } break; diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterAdmin.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterAdmin.java index 3c1a136ce11..10b71d78954 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterAdmin.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterAdmin.java @@ -251,7 +251,7 @@ public class TestRouterAdmin { // Verify starting condition MountTable entry = getMountTableEntry("/"); assertEquals( - Collections.singletonList(new RemoteLocation("ns0", "/")), + Collections.singletonList(new RemoteLocation("ns0", "/", "/")), entry.getDestinations()); // Edit the entry for / @@ -264,7 +264,7 @@ public class TestRouterAdmin { // Verify edited condition entry = getMountTableEntry("/"); assertEquals( - Collections.singletonList(new RemoteLocation("ns1", "/")), + Collections.singletonList(new RemoteLocation("ns1", "/", "/")), entry.getDestinations()); } diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterRpc.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterRpc.java index 414e11225e1..d5ec09359f4 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterRpc.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterRpc.java @@ -24,6 +24,7 @@ import static org.apache.hadoop.hdfs.server.federation.FederationTestUtils.delet import static org.apache.hadoop.hdfs.server.federation.FederationTestUtils.getFileStatus; import static org.apache.hadoop.hdfs.server.federation.FederationTestUtils.verifyFileExists; import static org.apache.hadoop.hdfs.server.federation.MiniRouterDFSCluster.TEST_STRING; +import static org.apache.hadoop.test.GenericTestUtils.assertExceptionContains; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -31,6 +32,7 @@ import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import java.io.IOException; import java.lang.reflect.Method; @@ -1022,6 +1024,52 @@ public class TestRouterRpc { assertEquals(nnSuccess, routerSuccess); } + @Test + public void testProxyExceptionMessages() throws IOException { + + // Install a mount point to a different path to check + MockResolver resolver = + (MockResolver)router.getRouter().getSubclusterResolver(); + String ns0 = cluster.getNameservices().get(0); + resolver.addLocation("/mnt", ns0, "/"); + + try { + FsPermission permission = new FsPermission("777"); + routerProtocol.mkdirs("/mnt/folder0/folder1", permission, false); + fail("mkdirs for non-existing parent folder should have failed"); + } catch (IOException ioe) { + assertExceptionContains("/mnt/folder0", ioe, + "Wrong path in exception for mkdirs"); + } + + try { + FsPermission permission = new FsPermission("777"); + routerProtocol.setPermission("/mnt/testfile.txt", permission); + fail("setPermission for non-existing file should have failed"); + } catch (IOException ioe) { + assertExceptionContains("/mnt/testfile.txt", ioe, + "Wrong path in exception for setPermission"); + } + + try { + FsPermission permission = new FsPermission("777"); + routerProtocol.mkdirs("/mnt/folder0/folder1", permission, false); + routerProtocol.delete("/mnt/folder0", false); + fail("delete for non-existing file should have failed"); + } catch (IOException ioe) { + assertExceptionContains("/mnt/folder0", ioe, + "Wrong path in exception for delete"); + } + + resolver.cleanRegistrations(); + + // Check corner cases + assertEquals( + "Parent directory doesn't exist: /ns1/a/a/b", + RouterRpcClient.processExceptionMsg( + "Parent directory doesn't exist: /a/a/b", "/a", "/ns1/a")); + } + @Test public void testErasureCoding() throws IOException { diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/records/TestMountTable.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/records/TestMountTable.java index d5fb9ba2283..43cf1766009 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/records/TestMountTable.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/records/TestMountTable.java @@ -50,8 +50,8 @@ public class TestMountTable { private static final String DST_PATH_1 = "/path/path2"; private static final List DST = new LinkedList<>(); static { - DST.add(new RemoteLocation(DST_NS_0, DST_PATH_0)); - DST.add(new RemoteLocation(DST_NS_1, DST_PATH_1)); + DST.add(new RemoteLocation(DST_NS_0, DST_PATH_0, SRC)); + DST.add(new RemoteLocation(DST_NS_1, DST_PATH_1, SRC)); } private static final Map DST_MAP = new LinkedHashMap<>(); static {