From cadd02ad1ddde1644f7241333c8ae446cb8c5f7f Mon Sep 17 00:00:00 2001
From: yliu <yliu@apache.org>
Date: Tue, 21 Jul 2015 09:18:43 +0800
Subject: [PATCH] HDFS-8794. Improve CorruptReplicasMap#corruptReplicasMap.
 (yliu)

---
 hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt   |  2 ++
 .../blockmanagement/CorruptReplicasMap.java   | 19 ++++++++++++++-----
 .../TestCorruptReplicaInfo.java               | 12 ++++++------
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
index c4ce0095747..f9b365b0816 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
+++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
@@ -386,6 +386,8 @@ Release 2.8.0 - UNRELEASED
     HDFS-7314. When the DFSClient lease cannot be renewed, abort open-for-write
     files rather than the entire DFSClient. (mingma)
 
+    HDFS-8794. Improve CorruptReplicasMap#corruptReplicasMap. (yliu)
+
   OPTIMIZATIONS
 
     HDFS-8026. Trace FSOutputSummer#writeChecksumChunks rather than
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/CorruptReplicasMap.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/CorruptReplicasMap.java
index fc2e23408f2..f83cbafda24 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/CorruptReplicasMap.java
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/CorruptReplicasMap.java
@@ -17,12 +17,19 @@
  */
 package org.apache.hadoop.hdfs.server.blockmanagement;
 
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.TreeMap;
+
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.hdfs.protocol.Block;
 import org.apache.hadoop.hdfs.server.namenode.NameNode;
 import org.apache.hadoop.ipc.Server;
 
-import java.util.*;
+import com.google.common.annotations.VisibleForTesting;
 
 /**
  * Stores information about all corrupt blocks in the File System.
@@ -46,8 +53,8 @@ public class CorruptReplicasMap{
     CORRUPTION_REPORTED  // client or datanode reported the corruption
   }
 
-  private final SortedMap<Block, Map<DatanodeDescriptor, Reason>> corruptReplicasMap =
-    new TreeMap<Block, Map<DatanodeDescriptor, Reason>>();
+  private final Map<Block, Map<DatanodeDescriptor, Reason>> corruptReplicasMap =
+    new HashMap<Block, Map<DatanodeDescriptor, Reason>>();
 
   /**
    * Mark the block belonging to datanode as corrupt.
@@ -181,13 +188,15 @@ public class CorruptReplicasMap{
    * @return Up to numExpectedBlocks blocks from startingBlockId if it exists
    *
    */
-  long[] getCorruptReplicaBlockIds(int numExpectedBlocks,
+  @VisibleForTesting
+  long[] getCorruptReplicaBlockIdsForTesting(int numExpectedBlocks,
                                    Long startingBlockId) {
     if (numExpectedBlocks < 0 || numExpectedBlocks > 100) {
       return null;
     }
     
-    Iterator<Block> blockIt = corruptReplicasMap.keySet().iterator();
+    Iterator<Block> blockIt = 
+        new TreeMap<>(corruptReplicasMap).keySet().iterator();
     
     // if the starting block id was specified, iterate over keys until
     // we find the matching block. If we find a matching block, break
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestCorruptReplicaInfo.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestCorruptReplicaInfo.java
index 21fb54e289c..4bdaaac60fc 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestCorruptReplicaInfo.java
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestCorruptReplicaInfo.java
@@ -73,9 +73,9 @@ public class TestCorruptReplicaInfo {
       
       // Make sure initial values are returned correctly
       assertEquals("Number of corrupt blocks must initially be 0", 0, crm.size());
-      assertNull("Param n cannot be less than 0", crm.getCorruptReplicaBlockIds(-1, null));
-      assertNull("Param n cannot be greater than 100", crm.getCorruptReplicaBlockIds(101, null));
-      long[] l = crm.getCorruptReplicaBlockIds(0, null);
+      assertNull("Param n cannot be less than 0", crm.getCorruptReplicaBlockIdsForTesting(-1, null));
+      assertNull("Param n cannot be greater than 100", crm.getCorruptReplicaBlockIdsForTesting(101, null));
+      long[] l = crm.getCorruptReplicaBlockIdsForTesting(0, null);
       assertNotNull("n = 0 must return non-null", l);
       assertEquals("n = 0 must return an empty list", 0, l.length);
 
@@ -118,14 +118,14 @@ public class TestCorruptReplicaInfo {
       
       assertTrue("First five block ids not returned correctly ",
                 Arrays.equals(new long[]{0,1,2,3,4},
-                              crm.getCorruptReplicaBlockIds(5, null)));
+                              crm.getCorruptReplicaBlockIdsForTesting(5, null)));
                               
-      LOG.info(crm.getCorruptReplicaBlockIds(10, 7L));
+      LOG.info(crm.getCorruptReplicaBlockIdsForTesting(10, 7L));
       LOG.info(block_ids.subList(7, 18));
 
       assertTrue("10 blocks after 7 not returned correctly ",
                 Arrays.equals(new long[]{8,9,10,11,12,13,14,15,16,17},
-                              crm.getCorruptReplicaBlockIds(10, 7L)));
+                              crm.getCorruptReplicaBlockIdsForTesting(10, 7L)));
       
   }