From b7ded466b00db0fe273058b844d56d810e0f8cc2 Mon Sep 17 00:00:00 2001 From: Jing Zhao Date: Wed, 27 Aug 2014 14:08:02 -0700 Subject: [PATCH] HDFS-6920. Archival Storage: check the storage type of delNodeHintStorage when deleting a replica. Contributed by Tsz Wo Nicholas Sze. --- .../server/blockmanagement/BlockManager.java | 28 +++++++++++++++---- .../BlockPlacementPolicyDefault.java | 11 ++++++-- .../blockmanagement/TestBlockManager.java | 18 +++++++++++- .../TestReplicationPolicy.java | 9 +++++- 4 files changed, 57 insertions(+), 9 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java index 52cd41c0ae7..389409b4b70 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java @@ -2771,12 +2771,9 @@ public class BlockManager { final DatanodeStorageInfo addedNodeStorage = DatanodeStorageInfo.getDatanodeStorageInfo(nonExcess, addedNode); while (nonExcess.size() - replication > 0) { - // check if we can delete delNodeHint final DatanodeStorageInfo cur; - if (firstOne && delNodeHintStorage != null - && (moreThanOne.contains(delNodeHintStorage) - || (addedNodeStorage != null - && !moreThanOne.contains(addedNodeStorage)))) { + if (useDelHint(firstOne, delNodeHintStorage, addedNodeStorage, + moreThanOne, excessTypes)) { cur = delNodeHintStorage; } else { // regular excessive replica removal cur = replicator.chooseReplicaToDelete(bc, b, replication, @@ -2806,6 +2803,27 @@ public class BlockManager { } } + /** Check if we can use delHint */ + static boolean useDelHint(boolean isFirst, DatanodeStorageInfo delHint, + DatanodeStorageInfo added, List moreThan1Racks, + List excessTypes) { + if (!isFirst) { + return false; // only consider delHint for the first case + } else if (delHint == null) { + return false; // no delHint + } else if (!excessTypes.remove(delHint.getStorageType())) { + return false; // delHint storage type is not an excess type + } else { + // check if removing delHint reduces the number of racks + if (moreThan1Racks.contains(delHint)) { + return true; // delHint and some other nodes are under the same rack + } else if (added != null && !moreThan1Racks.contains(added)) { + return true; // the added node adds a new rack + } + return false; // removing delHint reduces the number of racks; + } + } + private void addToExcessReplicate(DatanodeInfo dn, Block block) { assert namesystem.hasWriteLock(); LightWeightLinkedSet excessBlocks = excessReplicateMap.get(dn.getDatanodeUuid()); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockPlacementPolicyDefault.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockPlacementPolicyDefault.java index 392e3509c05..a711a09eebb 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockPlacementPolicyDefault.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockPlacementPolicyDefault.java @@ -792,8 +792,15 @@ public class BlockPlacementPolicyDefault extends BlockPlacementPolicy { minSpaceStorage = storage; } } - final DatanodeStorageInfo storage = oldestHeartbeatStorage != null? - oldestHeartbeatStorage : minSpaceStorage; + + final DatanodeStorageInfo storage; + if (oldestHeartbeatStorage != null) { + storage = oldestHeartbeatStorage; + } else if (minSpaceStorage != null) { + storage = minSpaceStorage; + } else { + return null; + } excessTypes.remove(storage.getStorageType()); return storage; } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java index 41af2370d14..0e13e832a72 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java @@ -38,6 +38,7 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hdfs.DFSConfigKeys; import org.apache.hadoop.hdfs.DFSTestUtil; import org.apache.hadoop.hdfs.HdfsConfiguration; +import org.apache.hadoop.hdfs.StorageType; import org.apache.hadoop.hdfs.protocol.Block; import org.apache.hadoop.hdfs.protocol.BlockListAsLongs; import org.apache.hadoop.hdfs.protocol.HdfsConstants; @@ -46,6 +47,7 @@ import org.apache.hadoop.hdfs.server.namenode.FSNamesystem; import org.apache.hadoop.hdfs.server.protocol.DatanodeRegistration; import org.apache.hadoop.hdfs.server.protocol.DatanodeStorage; import org.apache.hadoop.net.NetworkTopology; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.mockito.Mockito; @@ -599,5 +601,19 @@ public class TestBlockManager { new BlockListAsLongs(null, null)); assertEquals(1, ds.getBlockReportCount()); } -} + @Test + public void testUseDelHint() { + DatanodeStorageInfo delHint = new DatanodeStorageInfo( + DFSTestUtil.getLocalDatanodeDescriptor(), new DatanodeStorage("id")); + List moreThan1Racks = Arrays.asList(delHint); + List excessTypes = new ArrayList(); + + excessTypes.add(StorageType.DEFAULT); + Assert.assertTrue(BlockManager.useDelHint(true, delHint, null, + moreThan1Racks, excessTypes)); + excessTypes.add(StorageType.SSD); + Assert.assertFalse(BlockManager.useDelHint(true, delHint, null, + moreThan1Racks, excessTypes)); + } +} \ No newline at end of file diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicy.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicy.java index 892f849aced..b8f358f28bb 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicy.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicy.java @@ -20,6 +20,7 @@ package org.apache.hadoop.hdfs.server.blockmanagement; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; @@ -935,6 +936,12 @@ public class TestReplicationPolicy { assertEquals(2, first.size()); assertEquals(2, second.size()); List excessTypes = new ArrayList(); + { + // test returning null + excessTypes.add(StorageType.SSD); + assertNull(replicator.chooseReplicaToDelete( + null, null, (short)3, first, second, excessTypes)); + } excessTypes.add(StorageType.DEFAULT); DatanodeStorageInfo chosen = replicator.chooseReplicaToDelete( null, null, (short)3, first, second, excessTypes); @@ -950,7 +957,7 @@ public class TestReplicationPolicy { null, null, (short)2, first, second, excessTypes); assertEquals(chosen, storages[5]); } - + /** * This testcase tests whether the default value returned by * DFSUtil.getInvalidateWorkPctPerIteration() is positive,