HDFS-13845. RBF: The default MountTableResolver should fail resolving multi-destination paths. Contributed by yanghuafeng.

hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MountTableResolver.java

@@ -539,21 +539,28 @@ public class MountTableResolver
    * @param entry Mount table entry.
    * @return PathLocation containing the namespace, local path.
    */
-  private static PathLocation buildLocation(
+  private PathLocation buildLocation(
-      final String path, final MountTable entry) {
+      final String path, final MountTable entry) throws IOException {
-
     String srcPath = entry.getSourcePath();
     if (!path.startsWith(srcPath)) {
       LOG.error("Cannot build location, {} not a child of {}", path, srcPath);
       return null;
     }
+
+    List<RemoteLocation> dests = entry.getDestinations();
+    if (getClass() == MountTableResolver.class && dests.size() > 1) {
+      throw new IOException("Cannnot build location, "
+          + getClass().getSimpleName()
+          + " should not resolve multiple destinations for " + path);
+    }
+
     String remainingPath = path.substring(srcPath.length());
     if (remainingPath.startsWith(Path.SEPARATOR)) {
       remainingPath = remainingPath.substring(1);
     }
 
     List<RemoteLocation> locations = new LinkedList<>();
-    for (RemoteLocation oneDst : entry.getDestinations()) {
+    for (RemoteLocation oneDst : dests) {
       String nsId = oneDst.getNameserviceId();
       String dest = oneDst.getDest();
       String newPath = dest;

hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/resolver/TestMountTableResolver.java

@@ -79,6 +79,8 @@ public class TestMountTableResolver {
    * __usr
    * ____bin -> 2:/bin
    * __readonly -> 2:/tmp
+   * __multi -> 5:/dest1
+   *            6:/dest2
    *
    * @throws IOException If it cannot set the mount table.
    */
@@ -126,6 +128,12 @@ public class TestMountTableResolver {
     MountTable readOnlyEntry = MountTable.newInstance("/readonly", map);
     readOnlyEntry.setReadOnly(true);
     mountTable.addEntry(readOnlyEntry);
+
+    // /multi
+    map = getMountTableEntry("5", "/dest1");
+    map.put("6", "/dest2");
+    MountTable multiEntry = MountTable.newInstance("/multi", map);
+    mountTable.addEntry(multiEntry);
   }
 
   @Before
@@ -201,6 +209,17 @@ public class TestMountTableResolver {
     }
   }
 
+  @Test
+  public void testMuiltipleDestinations() throws IOException {
+    try {
+      mountTable.getDestinationForPath("/multi");
+      fail("The getDestinationForPath call should fail.");
+    } catch (IOException ioe) {
+      GenericTestUtils.assertExceptionContains(
+          "MountTableResolver should not resolve multiple destinations", ioe);
+    }
+  }
+
   private void compareLists(List<String> list1, String[] list2) {
     assertEquals(list1.size(), list2.length);
     for (String item : list2) {
@@ -236,8 +255,9 @@ public class TestMountTableResolver {
 
     // Check getting all mount points (virtual and real) beneath a path
     List<String> mounts = mountTable.getMountPoints("/");
-    assertEquals(4, mounts.size());
+    assertEquals(5, mounts.size());
-    compareLists(mounts, new String[] {"tmp", "user", "usr", "readonly"});
+    compareLists(mounts, new String[] {"tmp", "user", "usr",
+        "readonly", "multi"});
 
     mounts = mountTable.getMountPoints("/user");
     assertEquals(2, mounts.size());
@@ -263,6 +283,9 @@ public class TestMountTableResolver {
 
     mounts = mountTable.getMountPoints("/unknownpath");
     assertNull(mounts);
+
+    mounts = mountTable.getMountPoints("/multi");
+    assertEquals(0, mounts.size());
   }
 
   private void compareRecords(List<MountTable> list1, String[] list2) {
@@ -282,10 +305,10 @@ public class TestMountTableResolver {
 
     // Check listing the mount table records at or beneath a path
     List<MountTable> records = mountTable.getMounts("/");
-    assertEquals(9, records.size());
+    assertEquals(10, records.size());
     compareRecords(records, new String[] {"/", "/tmp", "/user", "/usr/bin",
         "user/a", "/user/a/demo/a", "/user/a/demo/b", "/user/b/file1.txt",
-        "readonly"});
+        "readonly", "multi"});
 
     records = mountTable.getMounts("/user");
     assertEquals(5, records.size());
@@ -305,6 +328,10 @@ public class TestMountTableResolver {
     assertEquals(1, records.size());
     compareRecords(records, new String[] {"/readonly"});
     assertTrue(records.get(0).isReadOnly());
+
+    records = mountTable.getMounts("/multi");
+    assertEquals(1, records.size());
+    compareRecords(records, new String[] {"/multi"});
   }
 
   @Test
@@ -313,7 +340,7 @@ public class TestMountTableResolver {
 
     // 3 mount points are present /tmp, /user, /usr
     compareLists(mountTable.getMountPoints("/"),
-        new String[] {"user", "usr", "tmp", "readonly"});
+        new String[] {"user", "usr", "tmp", "readonly", "multi"});
 
     // /tmp currently points to namespace 2
     assertEquals("2", mountTable.getDestinationForPath("/tmp/testfile.txt")
@@ -324,7 +351,7 @@ public class TestMountTableResolver {
 
     // Now 2 mount points are present /user, /usr
     compareLists(mountTable.getMountPoints("/"),
-        new String[] {"user", "usr", "readonly"});
+        new String[] {"user", "usr", "readonly", "multi"});
 
     // /tmp no longer exists, uses default namespace for mapping /
     assertEquals("1", mountTable.getDestinationForPath("/tmp/testfile.txt")
@@ -337,7 +364,7 @@ public class TestMountTableResolver {
 
     // 3 mount points are present /tmp, /user, /usr
     compareLists(mountTable.getMountPoints("/"),
-        new String[] {"user", "usr", "tmp", "readonly"});
+        new String[] {"user", "usr", "tmp", "readonly", "multi"});
 
     // /usr is virtual, uses namespace 1->/
     assertEquals("1", mountTable.getDestinationForPath("/usr/testfile.txt")
@@ -348,7 +375,7 @@ public class TestMountTableResolver {
 
     // Verify the remove failed
     compareLists(mountTable.getMountPoints("/"),
-        new String[] {"user", "usr", "tmp", "readonly"});
+        new String[] {"user", "usr", "tmp", "readonly", "multi"});
   }
 
   @Test
@@ -380,7 +407,7 @@ public class TestMountTableResolver {
 
     // Initial table loaded
     testDestination();
-    assertEquals(9, mountTable.getMounts("/").size());
+    assertEquals(10, mountTable.getMounts("/").size());
 
     // Replace table with /1 and /2
     List<MountTable> records = new ArrayList<>();

hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestDisableNameservices.java

@@ -21,6 +21,7 @@ import static org.apache.hadoop.hdfs.server.federation.FederationTestUtils.simul
 import static org.apache.hadoop.util.Time.monotonicNow;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.io.IOException;
 import java.util.Iterator;
@@ -43,13 +44,13 @@ import org.apache.hadoop.hdfs.server.federation.metrics.FederationMetrics;
 import org.apache.hadoop.hdfs.server.federation.resolver.MembershipNamenodeResolver;
 import org.apache.hadoop.hdfs.server.federation.resolver.MountTableManager;
 import org.apache.hadoop.hdfs.server.federation.resolver.MountTableResolver;
-import org.apache.hadoop.hdfs.server.federation.resolver.order.DestinationOrder;
 import org.apache.hadoop.hdfs.server.federation.store.DisabledNameserviceStore;
 import org.apache.hadoop.hdfs.server.federation.store.StateStoreService;
 import org.apache.hadoop.hdfs.server.federation.store.protocol.AddMountTableEntryRequest;
 import org.apache.hadoop.hdfs.server.federation.store.protocol.DisableNameserviceRequest;
 import org.apache.hadoop.hdfs.server.federation.store.records.MountTable;
 import org.apache.hadoop.hdfs.server.namenode.NameNode;
+import org.apache.hadoop.test.GenericTestUtils;
 import org.codehaus.jettison.json.JSONObject;
 import org.junit.After;
 import org.junit.AfterClass;
@@ -106,14 +107,18 @@ public class TestDisableNameservices {
     // Setup a mount table to map to the two namespaces
     MountTableManager mountTable = routerAdminClient.getMountTableManager();
     Map<String, String> destinations = new TreeMap<>();
-    destinations.put("ns0", "/");
+    destinations.put("ns0", "/dirns0");
-    destinations.put("ns1", "/");
+    MountTable newEntry = MountTable.newInstance("/dirns0", destinations);
-    MountTable newEntry = MountTable.newInstance("/", destinations);
-    newEntry.setDestOrder(DestinationOrder.RANDOM);
     AddMountTableEntryRequest request =
         AddMountTableEntryRequest.newInstance(newEntry);
     mountTable.addMountTableEntry(request);
 
+    destinations = new TreeMap<>();
+    destinations.put("ns1", "/dirns1");
+    newEntry = MountTable.newInstance("/dirns1", destinations);
+    request = AddMountTableEntryRequest.newInstance(newEntry);
+    mountTable.addMountTableEntry(request);
+
     // Refresh the cache in the Router
     Router router = routerContext.getRouter();
     MountTableResolver mountTableResolver =
@@ -122,9 +127,9 @@ public class TestDisableNameservices {
 
     // Add a folder to each namespace
     NamenodeContext nn0 = cluster.getNamenode("ns0", null);
-    nn0.getFileSystem().mkdirs(new Path("/dirns0"));
+    nn0.getFileSystem().mkdirs(new Path("/dirns0/0"));
     NamenodeContext nn1 = cluster.getNamenode("ns1", null);
-    nn1.getFileSystem().mkdirs(new Path("/dirns1"));
+    nn1.getFileSystem().mkdirs(new Path("/dirns1/1"));
   }
 
   @AfterClass
@@ -153,14 +158,12 @@ public class TestDisableNameservices {
 
   @Test
   public void testWithoutDisabling() throws IOException {
-
     // ns0 is slow and renewLease should take a long time
     long t0 = monotonicNow();
     routerProtocol.renewLease("client0");
     long t = monotonicNow() - t0;
     assertTrue("It took too little: " + t + "ms",
         t > TimeUnit.SECONDS.toMillis(1));
-
     // Return the results from all subclusters even if slow
     FileSystem routerFs = routerContext.getFileSystem();
     FileStatus[] filesStatus = routerFs.listStatus(new Path("/"));
@@ -171,7 +174,6 @@ public class TestDisableNameservices {
 
   @Test
   public void testDisabling() throws Exception {
-
     disableNameservice("ns0");
 
     // renewLease should be fast as we are skipping ns0
@@ -180,12 +182,20 @@ public class TestDisableNameservices {
     long t = monotonicNow() - t0;
     assertTrue("It took too long: " + t + "ms",
         t < TimeUnit.SECONDS.toMillis(1));
-
     // We should not report anything from ns0
     FileSystem routerFs = routerContext.getFileSystem();
-    FileStatus[] filesStatus = routerFs.listStatus(new Path("/"));
+    FileStatus[] filesStatus = null;
+    try {
+      routerFs.listStatus(new Path("/"));
+      fail("The listStatus call should fail.");
+    } catch (IOException ioe) {
+      GenericTestUtils.assertExceptionContains(
+          "No remote locations available", ioe);
+    }
+
+    filesStatus = routerFs.listStatus(new Path("/dirns1"));
     assertEquals(1, filesStatus.length);
-    assertEquals("dirns1", filesStatus[0].getPath().getName());
+    assertEquals("1", filesStatus[0].getPath().getName());
   }
 
   @Test