HDFS-12140. Remove BPOfferService lock contention to get block pool id. Contributed by Daryn Sharp.

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPOfferService.java

@@ -72,6 +72,7 @@ class BPOfferService {
   volatile DatanodeRegistration bpRegistration;
 
   private final String nameserviceId;
+  private volatile String bpId;
   private final DataNode dn;
 
   /**
@@ -184,6 +185,11 @@ String getNameserviceId() {
   }
 
   String getBlockPoolId(boolean quiet) {
+    // avoid lock contention unless the registration hasn't completed.
+    String id = bpId;
+    if (id != null) {
+      return id;
+    }
     readLock();
     try {
       if (bpNSInfo != null) {
@@ -205,7 +211,7 @@ String getBlockPoolId() {
   }
 
   boolean hasBlockPoolId() {
-    return getNamespaceInfo() != null;
+    return getBlockPoolId(true) != null;
   }
 
   NamespaceInfo getNamespaceInfo() {
@@ -217,6 +223,28 @@ NamespaceInfo getNamespaceInfo() {
     }
   }
 
+  @VisibleForTesting
+  NamespaceInfo setNamespaceInfo(NamespaceInfo nsInfo) throws IOException {
+    writeLock();
+    try {
+      NamespaceInfo old = bpNSInfo;
+      if (bpNSInfo != null && nsInfo != null) {
+        checkNSEquality(bpNSInfo.getBlockPoolID(), nsInfo.getBlockPoolID(),
+            "Blockpool ID");
+        checkNSEquality(bpNSInfo.getNamespaceID(), nsInfo.getNamespaceID(),
+            "Namespace ID");
+        checkNSEquality(bpNSInfo.getClusterID(), nsInfo.getClusterID(),
+            "Cluster ID");
+      }
+      bpNSInfo = nsInfo;
+      // cache the block pool id for lock-free access.
+      bpId = (nsInfo != null) ? nsInfo.getBlockPoolID() : null;
+      return old;
+    } finally {
+      writeUnlock();
+    }
+  }
+
   @Override
   public String toString() {
     readLock();
@@ -289,9 +317,10 @@ private void notifyNamenodeBlock(ExtendedBlock block, BlockStatus status,
   private void checkBlock(ExtendedBlock block) {
     Preconditions.checkArgument(block != null,
         "block is null");
-    Preconditions.checkArgument(block.getBlockPoolId().equals(getBlockPoolId()),
+    final String bpId = getBlockPoolId();
+    Preconditions.checkArgument(block.getBlockPoolId().equals(bpId),
         "block belongs to BP %s instead of BP %s",
-        block.getBlockPoolId(), getBlockPoolId());
+        block.getBlockPoolId(), bpId);
   }
 
   //This must be called only by blockPoolManager
@@ -337,8 +366,7 @@ void verifyAndSetNamespaceInfo(BPServiceActor actor, NamespaceInfo nsInfo)
     }
 
     try {
-      if (this.bpNSInfo == null) {
-        this.bpNSInfo = nsInfo;
+      if (setNamespaceInfo(nsInfo) == null) {
         boolean success = false;
 
         // Now that we know the namespace ID, etc, we can pass this to the DN.
@@ -352,16 +380,9 @@ void verifyAndSetNamespaceInfo(BPServiceActor actor, NamespaceInfo nsInfo)
             // The datanode failed to initialize the BP. We need to reset
             // the namespace info so that other BPService actors still have
             // a chance to set it, and re-initialize the datanode.
-            this.bpNSInfo = null;
+            setNamespaceInfo(null);
           }
         }
-      } else {
-        checkNSEquality(bpNSInfo.getBlockPoolID(), nsInfo.getBlockPoolID(),
-            "Blockpool ID");
-        checkNSEquality(bpNSInfo.getNamespaceID(), nsInfo.getNamespaceID(),
-            "Namespace ID");
-        checkNSEquality(bpNSInfo.getClusterID(), nsInfo.getClusterID(),
-            "Cluster ID");
       }
     } finally {
       writeUnlock();

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBPOfferService.java

@@ -225,6 +225,35 @@ public void testBasicFunctionality() throws Exception {
     }
   }
 
+  @Test
+  public void testLocklessBlockPoolId() throws Exception {
+    BPOfferService bpos = Mockito.spy(setupBPOSForNNs(mockNN1));
+
+    // bpNSInfo is not set, should take lock to check nsInfo.
+    assertNull(bpos.getBlockPoolId());
+    Mockito.verify(bpos).readLock();
+
+    // setting the bpNSInfo should cache the bp id, thus no locking.
+    Mockito.reset(bpos);
+    NamespaceInfo nsInfo = new NamespaceInfo(1, FAKE_CLUSTERID, FAKE_BPID, 0);
+    assertNull(bpos.setNamespaceInfo(nsInfo));
+    assertEquals(FAKE_BPID, bpos.getBlockPoolId());
+    Mockito.verify(bpos, Mockito.never()).readLock();
+
+    // clearing the bpNSInfo should clear the cached bp id, thus requiring
+    // locking to check the bpNSInfo.
+    Mockito.reset(bpos);
+    assertEquals(nsInfo, bpos.setNamespaceInfo(null));
+    assertNull(bpos.getBlockPoolId());
+    Mockito.verify(bpos).readLock();
+
+    // test setting it again.
+    Mockito.reset(bpos);
+    assertNull(bpos.setNamespaceInfo(nsInfo));
+    assertEquals(FAKE_BPID, bpos.getBlockPoolId());
+    Mockito.verify(bpos, Mockito.never()).readLock();
+  }
+
   /**
    * Test that DNA_INVALIDATE commands from the standby are ignored.
    */