HDFS-3798. Avoid throwing NPE when finalizeSegment() is called on invalid segment. Contributed by Todd Lipcon.

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

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

@@ -18,3 +18,5 @@ HDFS-3773. TestNNWithQJM fails after HDFS-3741. (atm)
 HDFS-3793. Implement genericized format() in QJM (todd)
 
 HDFS-3795. QJM: validate journal dir at startup (todd)
+
+HDFS-3798. Avoid throwing NPE when finalizeSegment() is called on invalid segment (todd)

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/qjournal/server/Journal.java

@@ -273,6 +273,11 @@ class Journal implements Closeable {
     }
     
     FileJournalManager.EditLogFile elf = fjm.getLogFile(startTxId);
+    if (elf == null) {
+      throw new IllegalStateException("No log file to finalize at " +
+          "transaction ID " + startTxId);
+    }
+
     if (elf.isInProgress()) {
       // TODO: this is slow to validate when in non-recovery cases
       // we already know the length here!
@@ -281,7 +286,7 @@ class Journal implements Closeable {
       elf.validateLog();
       
       Preconditions.checkState(elf.getLastTxId() == endTxId,
-          "Trying to finalize log %s-%s, but current state of log" +
+          "Trying to finalize log %s-%s, but current state of log " +
           "is %s", startTxId, endTxId, elf);
       fjm.finalizeLogSegment(startTxId, endTxId);
     } else {

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/qjournal/server/TestJournal.java

@@ -155,6 +155,60 @@ public class TestJournal {
     journal2.newEpoch(FAKE_NSINFO, 2);
   }
   
+  /**
+   * Test finalizing a segment after some batch of edits were missed.
+   * This should fail, since we validate the log before finalization.
+   */
+  @Test
+  public void testFinalizeWhenEditsAreMissed() throws Exception {
+    journal.newEpoch(FAKE_NSINFO, 1);
+    journal.startLogSegment(makeRI(1), 1);
+    journal.journal(makeRI(2), 1, 3,
+        QJMTestUtil.createTxnData(1, 3));
+    
+    // Try to finalize up to txn 6, even though we only wrote up to txn 3.
+    try {
+      journal.finalizeLogSegment(makeRI(3), 1, 6);
+      fail("did not fail to finalize");
+    } catch (IllegalStateException ise) {
+      GenericTestUtils.assertExceptionContains(
+          "but current state of log is", ise);
+    }
+    
+    // Check that, even if we re-construct the journal by scanning the
+    // disk, we don't allow finalizing incorrectly.
+    journal.close();
+    journal = new Journal(TEST_LOG_DIR, mockErrorReporter);
+    
+    try {
+      journal.finalizeLogSegment(makeRI(4), 1, 6);
+      fail("did not fail to finalize");
+    } catch (IllegalStateException ise) {
+      GenericTestUtils.assertExceptionContains(
+          "but current state of log is", ise);
+    }
+  }
+  
+  /**
+   * Ensure that finalizing a segment which doesn't exist throws the
+   * appropriate exception.
+   */
+  @Test
+  public void testFinalizeMissingSegment() throws Exception {
+    journal.newEpoch(FAKE_NSINFO, 1);
+    try {
+      journal.finalizeLogSegment(makeRI(1), 1000, 1001);
+      fail("did not fail to finalize");
+    } catch (IllegalStateException ise) {
+      GenericTestUtils.assertExceptionContains(
+          "No log file to finalize at transaction ID 1000", ise);
+    }
+  }
+  
+  private static RequestInfo makeRI(int serial) {
+    return new RequestInfo(JID, 1, serial);
+  }
+  
   @Test
   public void testNamespaceVerification() throws Exception {
     journal.newEpoch(FAKE_NSINFO, 1);