HDFS-4462. 2NN will fail to checkpoint after an HDFS upgrade from a pre-federation version of HDFS. Contributed by Aaron T. Myers.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1442375 13f79535-47bb-0310-9956-ffa450edef68

hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

@@ -753,6 +753,9 @@ Release 2.0.3-alpha - Unreleased
     HDFS-4445. All BKJM ledgers are not checked while tailing, So failover will fail.
     (Vinay via umamahesh)
 
+    HDFS-4462. 2NN will fail to checkpoint after an HDFS upgrade from a
+    pre-federation version of HDFS. (atm)
+
   BREAKDOWN OF HDFS-3077 SUBTASKS
 
     HDFS-3077. Quorum-based protocol for reading and writing edit logs.

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java

@@ -905,7 +905,7 @@ public abstract class Storage extends StorageInfo {
     props.setProperty("storageType", storageType.toString());
     props.setProperty("namespaceID", String.valueOf(namespaceID));
     // Set clusterID in version with federation support
-    if (LayoutVersion.supports(Feature.FEDERATION, layoutVersion)) {
+    if (versionSupportsFederation()) {
       props.setProperty("clusterID", clusterID);
     }
     props.setProperty("cTime", String.valueOf(cTime));

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/StorageInfo.java

@@ -18,6 +18,8 @@
 package org.apache.hadoop.hdfs.server.common;
 
 import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.hdfs.protocol.LayoutVersion;
+import org.apache.hadoop.hdfs.protocol.LayoutVersion.Feature;
 
 import com.google.common.base.Joiner;
 
@@ -77,6 +79,10 @@ public class StorageInfo {
     namespaceID = from.namespaceID;
     cTime = from.cTime;
   }
+
+  public boolean versionSupportsFederation() {
+    return LayoutVersion.supports(Feature.FEDERATION, layoutVersion);
+  }
   
   @Override
   public String toString() {

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/CheckpointSignature.java

@@ -123,6 +123,10 @@ public class CheckpointSignature extends StorageInfo
       blockpoolID.equals(si.getBlockPoolID());
   }
 
+  boolean namespaceIdMatches(FSImage si) {
+    return namespaceID == si.getStorage().namespaceID;
+  }
+
   void validateStorageInfo(FSImage si) throws IOException {
     if (!isSameCluster(si)
         || !storageVersionMatches(si.getStorage())) {

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java

@@ -587,7 +587,7 @@ public class NNStorage extends Storage implements Closeable,
     }
 
     // Set Block pool ID in version with federation support
-    if (LayoutVersion.supports(Feature.FEDERATION, layoutVersion)) {
+    if (versionSupportsFederation()) {
       String sbpid = props.getProperty("blockpoolID");
       setBlockPoolID(sd.getRoot(), sbpid);
     }
@@ -634,7 +634,7 @@ public class NNStorage extends Storage implements Closeable,
                            ) throws IOException {
     super.setPropertiesFromFields(props, sd);
     // Set blockpoolID in version with federation support
-    if (LayoutVersion.supports(Feature.FEDERATION, layoutVersion)) {
+    if (versionSupportsFederation()) {
       props.setProperty("blockpoolID", blockpoolID);
     }
   }

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/SecondaryNameNode.java

@@ -475,14 +475,20 @@ public class SecondaryNameNode implements Runnable {
     // Returns a token that would be used to upload the merged image.
     CheckpointSignature sig = namenode.rollEditLog();
     
-    if ((checkpointImage.getNamespaceID() == 0) ||
-        (sig.isSameCluster(checkpointImage) &&
+    boolean loadImage = false;
+    boolean isFreshCheckpointer = (checkpointImage.getNamespaceID() == 0);
+    boolean isSameCluster =
+        (dstStorage.versionSupportsFederation() && sig.isSameCluster(checkpointImage)) ||
+        (!dstStorage.versionSupportsFederation() && sig.namespaceIdMatches(checkpointImage));
+    if (isFreshCheckpointer ||
+        (isSameCluster &&
          !sig.storageVersionMatches(checkpointImage.getStorage()))) {
       // if we're a fresh 2NN, or if we're on the same cluster and our storage
       // needs an upgrade, just take the storage info from the server.
       dstStorage.setStorageInfo(sig);
       dstStorage.setClusterID(sig.getClusterID());
       dstStorage.setBlockPoolID(sig.getBlockpoolID());
+      loadImage = true;
     }
     sig.validateStorageInfo(checkpointImage);
 
@@ -492,7 +498,7 @@ public class SecondaryNameNode implements Runnable {
     RemoteEditLogManifest manifest =
       namenode.getEditLogManifest(sig.mostRecentCheckpointTxId + 1);
 
-    boolean loadImage = downloadCheckpointFiles(
+    loadImage |= downloadCheckpointFiles(
         fsName, checkpointImage, sig, manifest);   // Fetch fsimage and edits
     doMerge(sig, manifest, loadImage, checkpointImage, namesystem);
     

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSImageTestUtil.java

@@ -506,7 +506,11 @@ public abstract class FSImageTestUtil {
       props.load(fis);
       IOUtils.closeStream(fis);
   
-      props.setProperty(key, value);
+      if (value == null || value.isEmpty()) {
+        props.remove(key);
+      } else {
+        props.setProperty(key, value);
+      }
       
       out = new FileOutputStream(versionFile);
       props.store(out, null);

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestSecondaryNameNodeUpgrade.java

@@ -17,9 +17,12 @@
  */
 package org.apache.hadoop.hdfs.server.namenode;
 
+import com.google.common.collect.ImmutableMap;
+
 import java.io.File;
 import java.io.IOException;
 import java.util.List;
+import java.util.Map;
 
 import org.junit.Test;
 import org.junit.Before;
@@ -51,7 +54,7 @@ public class TestSecondaryNameNodeUpgrade {
     }
   }
 
-  private void doIt(String param, String val) throws IOException {
+  private void doIt(Map<String, String> paramsToCorrupt) throws IOException {
     MiniDFSCluster cluster = null;
     FileSystem fs = null;
     SecondaryNameNode snn = null;
@@ -76,8 +79,12 @@ public class TestSecondaryNameNodeUpgrade {
       snn.shutdown();
 
       for (File versionFile : versionFiles) {
-        System.out.println("Changing '" + param + "' to '" + val + "' in " + versionFile);
-        FSImageTestUtil.corruptVersionFile(versionFile, param, val);
+        for (Map.Entry<String, String> paramToCorrupt : paramsToCorrupt.entrySet()) {
+          String param = paramToCorrupt.getKey();
+          String val = paramToCorrupt.getValue();
+          System.out.println("Changing '" + param + "' to '" + val + "' in " + versionFile);
+          FSImageTestUtil.corruptVersionFile(versionFile, param, val);
+        }
       }
 
       snn = new SecondaryNameNode(conf);
@@ -94,13 +101,19 @@ public class TestSecondaryNameNodeUpgrade {
 
   @Test
   public void testUpgradeLayoutVersionSucceeds() throws IOException {
-    doIt("layoutVersion", "-39");
+    doIt(ImmutableMap.of("layoutVersion", "-39"));
+  }
+
+  @Test
+  public void testUpgradePreFedSucceeds() throws IOException {
+    doIt(ImmutableMap.of("layoutVersion", "-19", "clusterID", "",
+          "blockpoolID", ""));
   }
 
   @Test
   public void testChangeNsIDFails() throws IOException {
     try {
-      doIt("namespaceID", "2");
+      doIt(ImmutableMap.of("namespaceID", "2"));
       Assert.fail("Should throw InconsistentFSStateException");
     } catch(IOException e) {
       GenericTestUtils.assertExceptionContains("Inconsistent checkpoint fields", e);