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
2013-10-18 22:15:26 +00:00 · 2013-10-18 22:15:26 +00:00 · d61af97810
parent 1d96e36013
commit d61af97810
6 changed files with 220 additions and 30 deletions
--- 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)
--- 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 @@ public final class CacheManager {
    return nextEntryId++;
  }

-  private PathBasedCacheEntry findEntry(PathBasedCacheDirective directive) {
-    assert namesystem.hasReadOrWriteLock();
-    List<PathBasedCacheEntry> 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 final class CacheManager {
      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 final class CacheManager {
        continue;
      }
      if (filterPath != null &&
-          !directive.getPath().equals(filterPath)) {
+          !directive.getPath().toUri().getPath().equals(filterPath)) {
        continue;
      }
      if (pc.checkPermission(curEntry.getPool(), FsAction.READ)) {
--- 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 @@ public class FSNamesystem implements Namesystem, FSClusterStats,
    } finally {
      writeUnlock();
      if (isAuditEnabled() && isExternalInvocation()) {
-        logAuditEvent(success, "removePathBasedCacheDescriptors", null, null,
+        logAuditEvent(success, "removePathBasedCacheDescriptor", null, null,
            null);
      }
      RetryCache.setState(cacheEntry, success);
--- 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 class CacheAdmin extends Configured implements Tool {
    @Override
    public String getLongUsage() {
      TableListing listing = getOptionDescriptionListing();
-      listing.addRow("<id>", "The id of the cache directive to remove. " + 
+      listing.addRow("<id>", "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 class CacheAdmin extends Configured implements Tool {
    }
  }

+  private static class RemovePathBasedCacheDirectivesCommand implements Command {
+    @Override
+    public String getName() {
+      return "-removeDirectives";
+    }
+
+    @Override
+    public String getShortUsage() {
+      return "[" + getName() + " <path>]\n";
+    }
+
+    @Override
+    public String getLongUsage() {
+      TableListing listing = getOptionDescriptionListing();
+      listing.addRow("<path>", "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<String> 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<PathBasedCacheDescriptor> 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 class CacheAdmin extends Configured implements Tool {
  private static Command[] COMMANDS = {
    new AddPathBasedCacheDirectiveCommand(),
    new RemovePathBasedCacheDirectiveCommand(),
+    new RemovePathBasedCacheDirectivesCommand(),
    new ListPathBasedCacheDirectiveCommand(),
    new AddCachePoolCommand(),
    new ModifyCachePoolCommand(),
--- 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 class TestPathBasedCacheRequests {

    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 class TestPathBasedCacheRequests {

    RemoteIterator<PathBasedCacheDescriptor> 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 class TestPathBasedCacheRequests {
    }

    dfs.removePathBasedCacheDescriptor(alphaD);
+    dfs.removePathBasedCacheDescriptor(alphaD2);
    dfs.removePathBasedCacheDescriptor(deltaD);
    dfs.removePathBasedCacheDescriptor(relativeD);
    iter = dfs.listPathBasedCacheDescriptors(null, null);
@ -652,6 +654,7 @@ public class TestPathBasedCacheRequests {
  @Test(timeout=120000)
  public void testAddingPathBasedCacheDirectivesWhenCachingIsDisabled()
      throws Exception {
+    Assume.assumeTrue(canTestDatanodeCaching());
    HdfsConfiguration conf = createCachingConf();
    conf.setBoolean(DFS_NAMENODE_CACHING_ENABLED_KEY, false);
    MiniDFSCluster cluster =
--- 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 @@
      </comparators>
    </test>

+    <test> <!--Tested -->
+      <description>Testing listing directives filtered by pool</description>
+      <test-commands>
+        <cache-admin-command>-addPool pool1</cache-admin-command>
+        <cache-admin-command>-addPool pool2</cache-admin-command>
+        <cache-admin-command>-addDirective -path /foo -pool pool1</cache-admin-command>
+        <cache-admin-command>-addDirective -path /bar -pool pool1</cache-admin-command>
+        <cache-admin-command>-addDirective -path /baz -pool pool2</cache-admin-command>
+        <cache-admin-command>-addDirective -path /buz -pool pool2</cache-admin-command>
+        <cache-admin-command>-listDirectives -pool pool2</cache-admin-command>
+      </test-commands>
+      <cleanup-commands>
+        <cache-admin-command>-removePool pool1</cache-admin-command>
+        <cache-admin-command>-removePool pool2</cache-admin-command>
+      </cleanup-commands>
+      <comparators>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>Found 2 entries</expected-output>
+        </comparator>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>8   pool2  /baz</expected-output>
+        </comparator>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>9   pool2  /buz</expected-output>
+        </comparator>
+      </comparators>
+    </test>
+
+    <test> <!--Tested -->
+      <description>Testing listing directives filtered by path</description>
+      <test-commands>
+        <cache-admin-command>-addPool pool1</cache-admin-command>
+        <cache-admin-command>-addPool pool2</cache-admin-command>
+        <cache-admin-command>-addDirective -path /foo -pool pool1</cache-admin-command>
+        <cache-admin-command>-addDirective -path /bar -pool pool1</cache-admin-command>
+        <cache-admin-command>-addDirective -path /foo -pool pool2</cache-admin-command>
+        <cache-admin-command>-addDirective -path /bar -pool pool2</cache-admin-command>
+        <cache-admin-command>-listDirectives -path /foo</cache-admin-command>
+      </test-commands>
+      <cleanup-commands>
+        <cache-admin-command>-removePool pool1</cache-admin-command>
+        <cache-admin-command>-removePool pool2</cache-admin-command>
+      </cleanup-commands>
+      <comparators>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>Found 2 entries</expected-output>
+        </comparator>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>10  pool1  /foo</expected-output>
+        </comparator>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>12  pool2  /foo</expected-output>
+        </comparator>
+      </comparators>
+    </test>
+
+    <test> <!--Tested -->
+      <description>Testing listing directives filtered by path and pool</description>
+      <test-commands>
+        <cache-admin-command>-addPool pool1</cache-admin-command>
+        <cache-admin-command>-addPool pool2</cache-admin-command>
+        <cache-admin-command>-addDirective -path /foo -pool pool1</cache-admin-command>
+        <cache-admin-command>-addDirective -path /bar -pool pool1</cache-admin-command>
+        <cache-admin-command>-addDirective -path /foo -pool pool2</cache-admin-command>
+        <cache-admin-command>-addDirective -path /bar -pool pool2</cache-admin-command>
+        <cache-admin-command>-listDirectives -path /foo -pool pool2</cache-admin-command>
+      </test-commands>
+      <cleanup-commands>
+        <cache-admin-command>-removePool pool1</cache-admin-command>
+        <cache-admin-command>-removePool pool2</cache-admin-command>
+      </cleanup-commands>
+      <comparators>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>Found 1 entry</expected-output>
+        </comparator>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>16  pool2  /foo</expected-output>
+        </comparator>
+      </comparators>
+    </test>
+
+    <test> <!--Tested -->
+      <description>Testing removing a directive</description>
+      <test-commands>
+        <cache-admin-command>-addPool pool1</cache-admin-command>
+        <cache-admin-command>-addDirective -path /foo -pool pool1</cache-admin-command>
+        <cache-admin-command>-addDirective -path /bar -pool pool1</cache-admin-command>
+        <cache-admin-command>-removeDirective 18</cache-admin-command>
+        <cache-admin-command>-listDirectives</cache-admin-command>
+      </test-commands>
+      <cleanup-commands>
+        <cache-admin-command>-removePool pool1</cache-admin-command>
+      </cleanup-commands>
+      <comparators>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>Found 1 entry</expected-output>
+        </comparator>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>19  pool1  /bar</expected-output>
+        </comparator>
+      </comparators>
+    </test>
+
+    <test> <!--Tested -->
+      <description>Testing removing every directive for a path</description>
+      <test-commands>
+        <cache-admin-command>-addPool pool1</cache-admin-command>
+        <cache-admin-command>-addPool pool2</cache-admin-command>
+        <cache-admin-command>-addDirective -path /foo -pool pool1</cache-admin-command>
+        <cache-admin-command>-addDirective -path /foo -pool pool1</cache-admin-command>
+        <cache-admin-command>-addDirective -path /bar -pool pool1</cache-admin-command>
+        <cache-admin-command>-addDirective -path /foo -pool pool2</cache-admin-command>
+        <cache-admin-command>-addDirective -path /bar -pool pool2</cache-admin-command>
+        <cache-admin-command>-removeDirectives -path /foo</cache-admin-command>
+        <cache-admin-command>-listDirectives</cache-admin-command>
+      </test-commands>
+      <cleanup-commands>
+        <cache-admin-command>-removePool pool1</cache-admin-command>
+        <cache-admin-command>-removePool pool2</cache-admin-command>
+      </cleanup-commands>
+      <comparators>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>Found 2 entries</expected-output>
+        </comparator>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>22  pool1  /bar</expected-output>
+        </comparator>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>24  pool2  /bar</expected-output>
+        </comparator>
+      </comparators>
+    </test>
+
  </tests>
 </configuration>