From d61af9781086073152113d97106f708ea1cf6e8c Mon Sep 17 00:00:00 2001 From: Colin McCabe Date: Fri, 18 Oct 2013 22:15:26 +0000 Subject: [PATCH] HDFS-5203. Concurrent clients that add a cache directive on the same path may prematurely uncache each other. (Chris Nauroth via Colin Patrick McCabe) git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/HDFS-4949@1533651 13f79535-47bb-0310-9956-ffa450edef68 --- .../hadoop-hdfs/CHANGES-HDFS-4949.txt | 4 + .../hdfs/server/namenode/CacheManager.java | 24 +-- .../hdfs/server/namenode/FSNamesystem.java | 2 +- .../apache/hadoop/hdfs/tools/CacheAdmin.java | 63 +++++++- .../namenode/TestPathBasedCacheRequests.java | 11 +- .../src/test/resources/testCacheAdminConf.xml | 146 ++++++++++++++++++ 6 files changed, 220 insertions(+), 30 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4949.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4949.txt index 19825479b61..4c159ba6dd8 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4949.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4949.txt @@ -104,3 +104,7 @@ HDFS-4949 (Unreleased) HDFS-5388. Loading fsimage fails to find cache pools during namenode startup. (Chris Nauroth via Colin Patrick McCabe) + + HDFS-5203. Concurrent clients that add a cache directive on the same path + may prematurely uncache from each other. (Chris Nauroth via Colin Patrick + McCabe) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/CacheManager.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/CacheManager.java index da5b2f539ae..8931078f3d6 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/CacheManager.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/CacheManager.java @@ -266,21 +266,6 @@ private long getNextEntryId() throws IOException { return nextEntryId++; } - private PathBasedCacheEntry findEntry(PathBasedCacheDirective directive) { - assert namesystem.hasReadOrWriteLock(); - List existing = - entriesByPath.get(directive.getPath().toUri().getPath()); - if (existing == null) { - return null; - } - for (PathBasedCacheEntry entry : existing) { - if (entry.getPool().getPoolName().equals(directive.getPool())) { - return entry; - } - } - return null; - } - public PathBasedCacheDescriptor addDirective( PathBasedCacheDirective directive, FSPermissionChecker pc) throws IOException { @@ -302,13 +287,6 @@ public PathBasedCacheDescriptor addDirective( throw ioe; } - // Check if we already have this entry. - PathBasedCacheEntry existing = findEntry(directive); - if (existing != null) { - LOG.info("addDirective " + directive + ": there is an " + - "existing directive " + existing + " in this pool."); - return existing.getDescriptor(); - } // Add a new entry with the next available ID. PathBasedCacheEntry entry; try { @@ -405,7 +383,7 @@ public void removeDescriptor(long id, FSPermissionChecker pc) continue; } if (filterPath != null && - !directive.getPath().equals(filterPath)) { + !directive.getPath().toUri().getPath().equals(filterPath)) { continue; } if (pc.checkPermission(curEntry.getPool(), FsAction.READ)) { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java index 1ad779c8a63..1ad745607f9 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java @@ -6978,7 +6978,7 @@ void removePathBasedCacheDescriptor(Long id) throws IOException { } finally { writeUnlock(); if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(success, "removePathBasedCacheDescriptors", null, null, + logAuditEvent(success, "removePathBasedCacheDescriptor", null, null, null); } RetryCache.setState(cacheEntry, success); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/CacheAdmin.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/CacheAdmin.java index 0aa93169d80..10de3b37834 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/CacheAdmin.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/CacheAdmin.java @@ -207,10 +207,10 @@ public String getShortUsage() { @Override public String getLongUsage() { TableListing listing = getOptionDescriptionListing(); - listing.addRow("", "The id of the cache directive to remove. " + + listing.addRow("", "The id of the cache directive to remove. " + "You must have write permission on the pool of the " + "directive in order to remove it. To see a list " + - "of PathBasedCache directive IDs, use the -list command."); + "of PathBasedCache directive IDs, use the -listDirectives command."); return getShortUsage() + "\n" + "Remove a cache directive.\n\n" + listing.toString(); @@ -253,6 +253,64 @@ public int run(Configuration conf, List args) throws IOException { } } + private static class RemovePathBasedCacheDirectivesCommand implements Command { + @Override + public String getName() { + return "-removeDirectives"; + } + + @Override + public String getShortUsage() { + return "[" + getName() + " ]\n"; + } + + @Override + public String getLongUsage() { + TableListing listing = getOptionDescriptionListing(); + listing.addRow("", "The path of the cache directives to remove. " + + "You must have write permission on the pool of the directive in order " + + "to remove it. To see a list of cache directives, use the " + + "-listDirectives command."); + return getShortUsage() + "\n" + + "Remove every cache directive with the specified path.\n\n" + + listing.toString(); + } + + @Override + public int run(Configuration conf, List args) throws IOException { + String path = StringUtils.popOptionWithArgument("-path", args); + if (path == null) { + System.err.println("You must specify a path with -path."); + return 1; + } + if (!args.isEmpty()) { + System.err.println("Can't understand argument: " + args.get(0)); + System.err.println("Usage is " + getShortUsage()); + return 1; + } + DistributedFileSystem dfs = getDFS(conf); + RemoteIterator iter = + dfs.listPathBasedCacheDescriptors(null, new Path(path)); + int exitCode = 0; + while (iter.hasNext()) { + PathBasedCacheDescriptor entry = iter.next(); + try { + dfs.removePathBasedCacheDescriptor(entry); + System.out.println("Removed PathBasedCache directive " + + entry.getEntryId()); + } catch (RemovePathBasedCacheDescriptorException e) { + System.err.println(prettifyException(e)); + exitCode = 2; + } + } + if (exitCode == 0) { + System.out.println("Removed every PathBasedCache directive with path " + + path); + } + return exitCode; + } + } + private static class ListPathBasedCacheDirectiveCommand implements Command { @Override public String getName() { @@ -684,6 +742,7 @@ public int run(Configuration conf, List args) throws IOException { private static Command[] COMMANDS = { new AddPathBasedCacheDirectiveCommand(), new RemovePathBasedCacheDirectiveCommand(), + new RemovePathBasedCacheDirectivesCommand(), new ListPathBasedCacheDirectiveCommand(), new AddCachePoolCommand(), new ModifyCachePoolCommand(), diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestPathBasedCacheRequests.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestPathBasedCacheRequests.java index ce3713d39d1..53b7fba174c 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestPathBasedCacheRequests.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestPathBasedCacheRequests.java @@ -344,8 +344,9 @@ public void testAddRemoveDirectives() throws Exception { PathBasedCacheDescriptor alphaD = addAsUnprivileged(alpha); PathBasedCacheDescriptor alphaD2 = addAsUnprivileged(alpha); - assertEquals("Expected to get the same descriptor when re-adding" - + "an existing PathBasedCacheDirective", alphaD, alphaD2); + assertFalse("Expected to get unique descriptors when re-adding an " + + "existing PathBasedCacheDirective", + alphaD.getEntryId() == alphaD2.getEntryId()); PathBasedCacheDescriptor betaD = addAsUnprivileged(beta); try { @@ -404,11 +405,11 @@ public void testAddRemoveDirectives() throws Exception { RemoteIterator iter; iter = dfs.listPathBasedCacheDescriptors(null, null); - validateListAll(iter, alphaD, betaD, deltaD, relativeD); + validateListAll(iter, alphaD, alphaD2, betaD, deltaD, relativeD); iter = dfs.listPathBasedCacheDescriptors("pool3", null); Assert.assertFalse(iter.hasNext()); iter = dfs.listPathBasedCacheDescriptors("pool1", null); - validateListAll(iter, alphaD, deltaD, relativeD); + validateListAll(iter, alphaD, alphaD2, deltaD, relativeD); iter = dfs.listPathBasedCacheDescriptors("pool2", null); validateListAll(iter, betaD); @@ -437,6 +438,7 @@ public void testAddRemoveDirectives() throws Exception { } dfs.removePathBasedCacheDescriptor(alphaD); + dfs.removePathBasedCacheDescriptor(alphaD2); dfs.removePathBasedCacheDescriptor(deltaD); dfs.removePathBasedCacheDescriptor(relativeD); iter = dfs.listPathBasedCacheDescriptors(null, null); @@ -652,6 +654,7 @@ public void testWaitForCachedReplicas() throws Exception { @Test(timeout=120000) public void testAddingPathBasedCacheDirectivesWhenCachingIsDisabled() throws Exception { + Assume.assumeTrue(canTestDatanodeCaching()); HdfsConfiguration conf = createCachingConf(); conf.setBoolean(DFS_NAMENODE_CACHING_ENABLED_KEY, false); MiniDFSCluster cluster = diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testCacheAdminConf.xml b/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testCacheAdminConf.xml index 2e7506dcbef..b3edc17256a 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testCacheAdminConf.xml +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testCacheAdminConf.xml @@ -212,5 +212,151 @@ + + Testing listing directives filtered by pool + + -addPool pool1 + -addPool pool2 + -addDirective -path /foo -pool pool1 + -addDirective -path /bar -pool pool1 + -addDirective -path /baz -pool pool2 + -addDirective -path /buz -pool pool2 + -listDirectives -pool pool2 + + + -removePool pool1 + -removePool pool2 + + + + SubstringComparator + Found 2 entries + + + SubstringComparator + 8 pool2 /baz + + + SubstringComparator + 9 pool2 /buz + + + + + + Testing listing directives filtered by path + + -addPool pool1 + -addPool pool2 + -addDirective -path /foo -pool pool1 + -addDirective -path /bar -pool pool1 + -addDirective -path /foo -pool pool2 + -addDirective -path /bar -pool pool2 + -listDirectives -path /foo + + + -removePool pool1 + -removePool pool2 + + + + SubstringComparator + Found 2 entries + + + SubstringComparator + 10 pool1 /foo + + + SubstringComparator + 12 pool2 /foo + + + + + + Testing listing directives filtered by path and pool + + -addPool pool1 + -addPool pool2 + -addDirective -path /foo -pool pool1 + -addDirective -path /bar -pool pool1 + -addDirective -path /foo -pool pool2 + -addDirective -path /bar -pool pool2 + -listDirectives -path /foo -pool pool2 + + + -removePool pool1 + -removePool pool2 + + + + SubstringComparator + Found 1 entry + + + SubstringComparator + 16 pool2 /foo + + + + + + Testing removing a directive + + -addPool pool1 + -addDirective -path /foo -pool pool1 + -addDirective -path /bar -pool pool1 + -removeDirective 18 + -listDirectives + + + -removePool pool1 + + + + SubstringComparator + Found 1 entry + + + SubstringComparator + 19 pool1 /bar + + + + + + Testing removing every directive for a path + + -addPool pool1 + -addPool pool2 + -addDirective -path /foo -pool pool1 + -addDirective -path /foo -pool pool1 + -addDirective -path /bar -pool pool1 + -addDirective -path /foo -pool pool2 + -addDirective -path /bar -pool pool2 + -removeDirectives -path /foo + -listDirectives + + + -removePool pool1 + -removePool pool2 + + + + SubstringComparator + Found 2 entries + + + SubstringComparator + 22 pool1 /bar + + + SubstringComparator + 24 pool2 /bar + + + +