HDFS-2133. Address remaining TODOs and pre-merge cleanup on HDFS-1073 branch. Contributed by Todd Lipcon.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/HDFS-1073@1146856 13f79535-47bb-0310-9956-ffa450edef68

hdfs/CHANGES.HDFS-1073.txt

@@ -69,3 +69,5 @@ HDFS-2010. Fix NameNode to exit if all edit streams become inaccessible. (atm
 HDFS-2123. Checkpoint interval should be based on txn count, not size. (todd)
 HDFS-1979. Fix backupnode for new edits/image layout. (todd)
 HDFS-2101. Fix remaining unit tests for new storage filenames. (todd)
+HDFS-2133. Address remaining TODOs and pre-merge cleanup on HDFS-1073 branch.
+           (todd)

hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileOutputStream.java

@@ -137,9 +137,6 @@ class EditLogFileOutputStream extends EditLogOutputStream {
       throw new IOException("Trying to use aborted output stream");
     }
 
-    setReadyToFlush();
-    flush();
-
     // close should have been called after all pending transactions
     // have been flushed & synced.
     // if already closed, just skip

hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImage.java

@@ -560,7 +560,6 @@ public class FSImage implements Closeable {
    * file.
    */
   void reloadFromImageFile(File file) throws IOException {
-    // TODO: namesystem.close(); ??
     namesystem.dir.reset();
 
     LOG.debug("Reloading namespace from " + file);
@@ -603,8 +602,6 @@ public class FSImage implements Closeable {
     //
     StorageDirectory sdForProperties =
       loadPlan.getStorageDirectoryForProperties();
-    // TODO need to discuss what the correct logic is for determing which
-    // storage directory to read properties from
     sdForProperties.read();
     File imageFile = loadPlan.getImageFile();
 
@@ -798,8 +795,7 @@ public class FSImage implements Closeable {
     long imageTxId = editLog.getLastWrittenTxId();
     try {
       saveFSImageInAllDirs(imageTxId);
-      storage.writeAll(); // TODO is this a good spot for this?
-      
+      storage.writeAll();
     } finally {
       if (editLogWasOpen) {
         editLog.startLogSegment(imageTxId + 1, true);
@@ -934,33 +930,23 @@ public class FSImage implements Closeable {
             + " role " + bnReg.getRole() + ": checkpoint is not allowed.";
     else if(bnReg.getLayoutVersion() < storage.getLayoutVersion()
         || (bnReg.getLayoutVersion() == storage.getLayoutVersion()
-            && bnReg.getCTime() > storage.getCTime())
-        || (bnReg.getLayoutVersion() == storage.getLayoutVersion()
-            && bnReg.getCTime() == storage.getCTime()
-            && bnReg.getCheckpointTxId() > storage.getMostRecentCheckpointTxId()))
+            && bnReg.getCTime() > storage.getCTime()))
       // remote node has newer image age
       msg = "Name node " + bnReg.getAddress()
             + " has newer image layout version: LV = " +bnReg.getLayoutVersion()
             + " cTime = " + bnReg.getCTime()
-            + " checkpointTxId = " + bnReg.getCheckpointTxId()
             + ". Current version: LV = " + storage.getLayoutVersion()
-            + " cTime = " + storage.getCTime()
-            + " checkpointTxId = " + storage.getMostRecentCheckpointTxId();
+            + " cTime = " + storage.getCTime();
     if(msg != null) {
       LOG.error(msg);
       return new NamenodeCommand(NamenodeProtocol.ACT_SHUTDOWN);
     }
-    boolean isImgObsolete = true;
-    if(bnReg.getLayoutVersion() == storage.getLayoutVersion()
-        && bnReg.getCTime() == storage.getCTime()
-        && bnReg.getCheckpointTxId() == storage.getMostRecentCheckpointTxId())
-      isImgObsolete = false;
     boolean needToReturnImg = true;
     if(storage.getNumStorageDirs(NameNodeDirType.IMAGE) == 0)
       // do not return image if there are no image directories
       needToReturnImg = false;
     CheckpointSignature sig = rollEditLog();
-    return new CheckpointCommand(sig, isImgObsolete, needToReturnImg);
+    return new CheckpointCommand(sig, needToReturnImg);
   }
 
   /**

hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSImageTransactionalStorageInspector.java

@@ -303,7 +303,9 @@ class FSImageTransactionalStorageInspector extends FSImageStorageInspector {
 
   @Override
   public boolean needToSave() {
-    return false; // TODO do we need to do this ever?
+    // No need to save at startup - it's OK to have outstanding
+    // logs - better to wait until next 2NN-based checkpoint
+    return false;
   }
   
   

hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/JournalManager.java

@@ -30,10 +30,15 @@ import org.apache.hadoop.hdfs.server.namenode.NNStorageArchivalManager.StorageAr
  */
 public interface JournalManager {
   /**
-   * TODO
+   * Begin writing to a new segment of the log stream, which starts at
+   * the given transaction ID.
    */
   EditLogOutputStream startLogSegment(long txId) throws IOException;
-  
+
+  /**
+   * Mark the log segment that spans from firstTxId to lastTxId
+   * as finalized and complete.
+   */
   void finalizeLogSegment(long firstTxId, long lastTxId) throws IOException;
 
   /**

hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java

@@ -217,10 +217,7 @@ public class NNStorage extends Storage implements Closeable {
 
   /**
    * See if any of removed storages is "writable" again, and can be returned
-   * into service. If saveNamespace is set, then this method is being
-   * called from saveNamespace.
-   *
-   * @param saveNamespace Whether method is being called from saveNamespace()
+   * into service.
    */
   void attemptRestoreRemovedStorage() {
     // if directory is "alive" - copy the images there...

hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java

@@ -388,8 +388,7 @@ public class NameNode implements NamenodeProtocols, FSConstants {
     nodeRegistration = new NamenodeRegistration(
         getHostPortString(rpcAddress),
         getHostPortString(httpAddress),
-        getFSImage().getStorage(), getRole(),
-        getFSImage().getStorage().getMostRecentCheckpointTxId());
+        getFSImage().getStorage(), getRole());
     return nodeRegistration;
   }
 

hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/CheckpointCommand.java

@@ -47,19 +47,16 @@ import org.apache.hadoop.hdfs.server.namenode.CheckpointSignature;
 @InterfaceStability.Evolving
 public class CheckpointCommand extends NamenodeCommand {
   private CheckpointSignature cSig;
-  private boolean isImageObsolete;
   private boolean needToReturnImage;
 
   public CheckpointCommand() {
-    this(null, false, false);
+    this(null, false);
   }
 
   public CheckpointCommand(CheckpointSignature sig,
-                           boolean isImgObsolete,
                            boolean needToReturnImg) {
     super(NamenodeProtocol.ACT_CHECKPOINT);
     this.cSig = sig;
-    this.isImageObsolete = isImgObsolete;
     this.needToReturnImage = needToReturnImg;
   }
 
@@ -71,16 +68,6 @@ public class CheckpointCommand extends NamenodeCommand {
     return cSig;
   }
 
-  /**
-   * Indicates whether current backup image is obsolete, and therefore
-   * need to be discarded?
-   * 
-   * @return true if current image should be discarded.
-   */
-  public boolean isImageObsolete() {
-    return isImageObsolete;
-  }
-
   /**
    * Indicates whether the new checkpoint image needs to be transfered 
    * back to the name-node after the checkpoint is done.
@@ -104,7 +91,6 @@ public class CheckpointCommand extends NamenodeCommand {
   public void write(DataOutput out) throws IOException {
     super.write(out);
     cSig.write(out);
-    out.writeBoolean(isImageObsolete);
     out.writeBoolean(needToReturnImage);
   }
   
@@ -112,7 +98,6 @@ public class CheckpointCommand extends NamenodeCommand {
     super.readFields(in);
     cSig = new CheckpointSignature();
     cSig.readFields(in);
-    isImageObsolete = in.readBoolean();
     needToReturnImage = in.readBoolean();
   }
 }

hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/NamenodeRegistration.java

@@ -44,10 +44,6 @@ implements NodeRegistration {
   String rpcAddress;          // RPC address of the node
   String httpAddress;         // HTTP address of the node
   NamenodeRole role;          // node role
-  
-  // TODO: is the below used by anything?
-  long checkpointTxId = FSConstants.INVALID_TXID;
-                              // the age of the image
 
   public NamenodeRegistration() {
     super();
@@ -56,14 +52,12 @@ implements NodeRegistration {
   public NamenodeRegistration(String address,
                               String httpAddress,
                               StorageInfo storageInfo,
-                              NamenodeRole role,
-                              long checkpointTxId) {
+                              NamenodeRole role) {
     super();
     this.rpcAddress = address;
     this.httpAddress = httpAddress;
     this.setStorageInfo(storageInfo);
     this.role = role;
-    this.checkpointTxId= checkpointTxId;
   }
 
   @Override // NodeRegistration
@@ -100,13 +94,6 @@ implements NodeRegistration {
     return role.equals(that);
   }
 
-  /**
-   * Get the age of the image.
-   */
-  public long getCheckpointTxId() {
-    return checkpointTxId;
-  }
-
   /////////////////////////////////////////////////
   // Writable
   /////////////////////////////////////////////////
@@ -124,7 +111,6 @@ implements NodeRegistration {
     Text.writeString(out, httpAddress);
     Text.writeString(out, role.name());
     super.write(out);
-    out.writeLong(checkpointTxId);
   }
 
   @Override // Writable
@@ -133,6 +119,5 @@ implements NodeRegistration {
     httpAddress = Text.readString(in);
     role = NamenodeRole.valueOf(Text.readString(in));
     super.readFields(in);
-    checkpointTxId = in.readLong();
   }
 }