From 0f9b8d7a753ad41b7ee7dfe3afaf34bddcbd94a8 Mon Sep 17 00:00:00 2001 From: Ayush Saxena Date: Mon, 8 Apr 2019 21:43:59 +0530 Subject: [PATCH] HDFS-14369. RBF: Fix trailing / for webhdfs. Contributed by Akira Ajisaka. --- .../resolver/MountTableResolver.java | 14 +- .../hdfs/tools/federation/RouterAdmin.java | 16 ++- .../resolver/TestMountTableResolver.java | 123 ++++++++++++++++-- .../router/TestRouterMountTable.java | 32 +++++ 4 files changed, 160 insertions(+), 25 deletions(-) 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 da585515c3..03b051db34 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 @@ -57,6 +57,7 @@ import org.apache.hadoop.hdfs.server.federation.store.StateStoreUnavailableExcep import org.apache.hadoop.hdfs.server.federation.store.protocol.GetMountTableEntriesRequest; import org.apache.hadoop.hdfs.server.federation.store.protocol.GetMountTableEntriesResponse; import org.apache.hadoop.hdfs.server.federation.store.records.MountTable; +import org.apache.hadoop.hdfs.tools.federation.RouterAdmin; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -399,12 +400,13 @@ public class MountTableResolver /** * Build the path location to insert into the cache atomically. It must hold * the read lock. - * @param path Path to check/insert. + * @param str Path to check/insert. * @return New remote location. * @throws IOException If it cannot find the location. */ - public PathLocation lookupLocation(final String path) throws IOException { + public PathLocation lookupLocation(final String str) throws IOException { PathLocation ret = null; + final String path = RouterAdmin.normalizeFileSystemPath(str); MountTable entry = findDeepest(path); if (entry != null) { ret = buildLocation(path, entry); @@ -432,12 +434,13 @@ public class MountTableResolver */ public MountTable getMountPoint(final String path) throws IOException { verifyMountTable(); - return findDeepest(path); + return findDeepest(RouterAdmin.normalizeFileSystemPath(path)); } @Override - public List getMountPoints(final String path) throws IOException { + public List getMountPoints(final String str) throws IOException { verifyMountTable(); + final String path = RouterAdmin.normalizeFileSystemPath(str); Set children = new TreeSet<>(); readLock.lock(); @@ -493,8 +496,7 @@ public class MountTableResolver */ public List getMounts(final String path) throws IOException { verifyMountTable(); - - return getTreeValues(path, false); + return getTreeValues(RouterAdmin.normalizeFileSystemPath(path), false); } /** diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java index 9d03a4485b..8f6d917d3a 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java @@ -26,12 +26,12 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.regex.Pattern; import org.apache.hadoop.classification.InterfaceAudience.Private; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.conf.Configured; import org.apache.hadoop.fs.CommonConfigurationKeys; -import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.hdfs.DFSConfigKeys; import org.apache.hadoop.hdfs.HdfsConfiguration; @@ -93,6 +93,9 @@ public class RouterAdmin extends Configured implements Tool { private RouterClient client; + /** Pre-compiled regular expressions to detect duplicated slashes. */ + private static final Pattern SLASHES = Pattern.compile("/+"); + public static void main(String[] argv) throws Exception { Configuration conf = new HdfsConfiguration(); RouterAdmin admin = new RouterAdmin(conf); @@ -1062,12 +1065,15 @@ public class RouterAdmin extends Configured implements Tool { /** * Normalize a path for that filesystem. * - * @param path Path to normalize. + * @param str Path to normalize. The path doesn't have scheme or authority. * @return Normalized path. */ - private static String normalizeFileSystemPath(final String path) { - Path normalizedPath = new Path(path); - return normalizedPath.toString(); + public static String normalizeFileSystemPath(final String str) { + String path = SLASHES.matcher(str).replaceAll("/"); + if (path.length() > 1 && path.endsWith("/")) { + path = path.substring(0, path.length()-1); + } + return path; } /** diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/resolver/TestMountTableResolver.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/resolver/TestMountTableResolver.java index 14ccb6112b..ada2815872 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/resolver/TestMountTableResolver.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/resolver/TestMountTableResolver.java @@ -184,6 +184,23 @@ public class TestMountTableResolver { } + @Test + public void testDestinationOfConsecutiveSlash() throws IOException { + // Check files + assertEquals("1->/tesfile1.txt", + mountTable.getDestinationForPath("//tesfile1.txt///").toString()); + + assertEquals("3->/user/testfile2.txt", + mountTable.getDestinationForPath("/user///testfile2.txt").toString()); + + assertEquals("2->/user/test/testfile3.txt", + mountTable.getDestinationForPath("///user/a/testfile3.txt").toString()); + + assertEquals("3->/user/b/testfile4.txt", + mountTable.getDestinationForPath("/user/b/testfile4.txt//").toString()); + } + + @Test public void testDefaultNameServiceEnable() throws IOException { assertTrue(mountTable.isDefaultNSEnable()); @@ -232,62 +249,120 @@ public class TestMountTableResolver { // Check get the mount table entry for a path MountTable mtEntry; mtEntry = mountTable.getMountPoint("/"); - assertTrue(mtEntry.getSourcePath().equals("/")); + assertEquals("/", mtEntry.getSourcePath()); mtEntry = mountTable.getMountPoint("/user"); - assertTrue(mtEntry.getSourcePath().equals("/user")); + assertEquals("/user", mtEntry.getSourcePath()); mtEntry = mountTable.getMountPoint("/user/a"); - assertTrue(mtEntry.getSourcePath().equals("/user/a")); + assertEquals("/user/a", mtEntry.getSourcePath()); mtEntry = mountTable.getMountPoint("/user/a/"); - assertTrue(mtEntry.getSourcePath().equals("/user/a")); + assertEquals("/user/a", mtEntry.getSourcePath()); mtEntry = mountTable.getMountPoint("/user/a/11"); - assertTrue(mtEntry.getSourcePath().equals("/user/a")); + assertEquals("/user/a", mtEntry.getSourcePath()); mtEntry = mountTable.getMountPoint("/user/a1"); - assertTrue(mtEntry.getSourcePath().equals("/user")); + assertEquals("/user", mtEntry.getSourcePath()); + } + + @Test + public void testGetMountPointOfConsecutiveSlashes() throws IOException { + // Check get the mount table entry for a path + MountTable mtEntry; + mtEntry = mountTable.getMountPoint("///"); + assertEquals("/", mtEntry.getSourcePath()); + + mtEntry = mountTable.getMountPoint("///user//"); + assertEquals("/user", mtEntry.getSourcePath()); + + mtEntry = mountTable.getMountPoint("/user///a"); + assertEquals("/user/a", mtEntry.getSourcePath()); + + mtEntry = mountTable.getMountPoint("/user/a////"); + assertEquals("/user/a", mtEntry.getSourcePath()); + + mtEntry = mountTable.getMountPoint("///user/a/11//"); + assertEquals("/user/a", mtEntry.getSourcePath()); + + mtEntry = mountTable.getMountPoint("/user///a1///"); + assertEquals("/user", mtEntry.getSourcePath()); + } + + @Test + public void testTrailingSlashInInputPath() throws IOException { + // Check mount points beneath the path with trailing slash. + getMountPoints(true); } @Test public void testGetMountPoints() throws IOException { + // Check mount points beneath the path without trailing slash. + getMountPoints(false); + } + private void getMountPoints(boolean trailingSlash) throws IOException { // Check getting all mount points (virtual and real) beneath a path List mounts = mountTable.getMountPoints("/"); assertEquals(5, mounts.size()); compareLists(mounts, new String[] {"tmp", "user", "usr", "readonly", "multi"}); - mounts = mountTable.getMountPoints("/user"); + String path = trailingSlash ? "/user/" : "/user"; + mounts = mountTable.getMountPoints(path); assertEquals(2, mounts.size()); compareLists(mounts, new String[] {"a", "b"}); - mounts = mountTable.getMountPoints("/user/a"); + path = trailingSlash ? "/user/a/" : "/user/a"; + mounts = mountTable.getMountPoints(path); assertEquals(1, mounts.size()); compareLists(mounts, new String[] {"demo"}); - mounts = mountTable.getMountPoints("/user/a/demo"); + path = trailingSlash ? "/user/a/demo/" : "/user/a/demo"; + mounts = mountTable.getMountPoints(path); assertEquals(1, mounts.size()); compareLists(mounts, new String[] {"test"}); - mounts = mountTable.getMountPoints("/user/a/demo/test"); + path = trailingSlash ? "/user/a/demo/test/" : "/user/a/demo/test"; + mounts = mountTable.getMountPoints(path); assertEquals(2, mounts.size()); compareLists(mounts, new String[] {"a", "b"}); - mounts = mountTable.getMountPoints("/tmp"); + path = trailingSlash ? "/tmp/" : "/tmp"; + mounts = mountTable.getMountPoints(path); assertEquals(0, mounts.size()); - mounts = mountTable.getMountPoints("/t"); + path = trailingSlash ? "/t/" : "/t"; + mounts = mountTable.getMountPoints(path); assertNull(mounts); - mounts = mountTable.getMountPoints("/unknownpath"); + path = trailingSlash ? "/unknownpath/" : "/unknownpath"; + mounts = mountTable.getMountPoints(path); assertNull(mounts); - mounts = mountTable.getMountPoints("/multi"); + path = trailingSlash ? "/multi/" : "/multi"; + mounts = mountTable.getMountPoints(path); assertEquals(0, mounts.size()); } + @Test + public void testSuccessiveSlashesInInputPath() throws IOException { + // Check getting all mount points (virtual and real) beneath a path + List mounts = mountTable.getMountPoints("///"); + assertEquals(5, mounts.size()); + compareLists(mounts, new String[] {"tmp", "user", "usr", + "readonly", "multi"}); + String path = "///user//"; + mounts = mountTable.getMountPoints(path); + assertEquals(2, mounts.size()); + compareLists(mounts, new String[] {"a", "b"}); + path = "/user///a"; + mounts = mountTable.getMountPoints(path); + assertEquals(1, mounts.size()); + compareLists(mounts, new String[] {"demo"}); + } + private void compareRecords(List list1, String[] list2) { assertEquals(list1.size(), list2.length); for (String item : list2) { @@ -334,6 +409,26 @@ public class TestMountTableResolver { compareRecords(records, new String[] {"/multi"}); } + @Test + public void testGetMountsOfConsecutiveSlashes() throws IOException { + // Check listing the mount table records at or beneath a path + List records = mountTable.getMounts("///"); + 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", "multi"}); + + records = mountTable.getMounts("/user///"); + assertEquals(5, records.size()); + compareRecords(records, new String[] {"/user", "/user/a/demo/a", + "/user/a/demo/b", "user/a", "/user/b/file1.txt"}); + + records = mountTable.getMounts("///user///a"); + assertEquals(3, records.size()); + compareRecords(records, + new String[] {"/user/a/demo/a", "/user/a/demo/b", "/user/a"}); + } + @Test public void testRemoveSubTree() throws UnsupportedOperationException, IOException { diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterMountTable.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterMountTable.java index 4f6f702d9a..24dfc3fd31 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterMountTable.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterMountTable.java @@ -442,4 +442,36 @@ public class TestRouterMountTable { "Directory/File does not exist /mount/file", () -> routerFs.setOwner(new Path("/mount/file"), "user", "group")); } + + /** + * Regression test for HDFS-14369. + * Verify that getListing works with the path with trailing slash. + */ + @Test + public void testGetListingWithTrailingSlash() throws IOException { + try { + // Add mount table entry + MountTable addEntry = MountTable.newInstance("/testlist", + Collections.singletonMap("ns0", "/testlist")); + assertTrue(addMountTable(addEntry)); + addEntry = MountTable.newInstance("/testlist/tmp0", + Collections.singletonMap("ns0", "/testlist/tmp0")); + assertTrue(addMountTable(addEntry)); + addEntry = MountTable.newInstance("/testlist/tmp1", + Collections.singletonMap("ns1", "/testlist/tmp1")); + assertTrue(addMountTable(addEntry)); + + nnFs0.mkdirs(new Path("/testlist/tmp0")); + nnFs1.mkdirs(new Path("/testlist/tmp1")); + // Fetch listing + DirectoryListing list = routerProtocol.getListing( + "/testlist/", HdfsFileStatus.EMPTY_NAME, false); + HdfsFileStatus[] statuses = list.getPartialListing(); + // should return tmp0 and tmp1 + assertEquals(2, statuses.length); + } finally { + nnFs0.delete(new Path("/testlist/tmp0"), true); + nnFs1.delete(new Path("/testlist/tmp1"), true); + } + } } \ No newline at end of file