You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/04/07 10:22:00 UTC
[GitHub] [hadoop] tasanuma commented on a diff in pull request #4126: HDFS-16456. EC: Decommission a rack with only on dn will fail when the rack number is equal with replication
tasanuma commented on code in PR #4126:
URL: https://github.com/apache/hadoop/pull/4126#discussion_r844954667
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestBlockPlacementPolicyRackFaultTolerant.java:
##########
@@ -167,6 +179,107 @@ private void doTestChooseTargetSpecialCase() throws Exception {
}
}
+ /**
+ * Verify decommission a dn which is an only node in its rack.
+ */
+ public void testPlacementWithOnlyOneNodeInRackDecommission() throws Exception {
Review Comment:
Instead of adding `testPlacementWithOnlyOneNodeInRackDecommission()` to `testChooseTarget()`, it would be better to create `testPlacementWithOnlyOneNodeInRackDecommission()` as one unit test method.
```suggestion
@Test
public void testPlacementWithOnlyOneNodeInRackDecommission() throws Exception {
```
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetworkTopology.java:
##########
@@ -101,6 +101,13 @@ protected NetworkTopology init(InnerNode.Factory factory) {
private int depthOfAllLeaves = -1;
/** rack counter */
protected int numOfRacks = 0;
+ /** empty rack map, rackname->nodenumber. */
+ private HashMap<String, Set<String>> emptyRackMap =
Review Comment:
This is a rack map to calculate the number of empty racks. So I prefer to use the name `rackMap`.
```suggestion
private HashMap<String, Set<String>> rackMap =
```
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestBlockPlacementPolicyRackFaultTolerant.java:
##########
@@ -167,6 +179,107 @@ private void doTestChooseTargetSpecialCase() throws Exception {
}
}
+ /**
+ * Verify decommission a dn which is an only node in its rack.
+ */
+ public void testPlacementWithOnlyOneNodeInRackDecommission() throws Exception {
+ Configuration conf = new HdfsConfiguration();
+ final String[] racks = {"/RACK0", "/RACK0", "/RACK2", "/RACK3", "/RACK4", "/RACK5", "/RACK2"};
+ final String[] hosts = {"/host0", "/host1", "/host2", "/host3", "/host4", "/host5", "/host6"};
+
+ // enables DFSNetworkTopology
+ conf.setClass(DFSConfigKeys.DFS_BLOCK_REPLICATOR_CLASSNAME_KEY,
+ BlockPlacementPolicyRackFaultTolerant.class,
+ BlockPlacementPolicy.class);
+ conf.setBoolean(DFSConfigKeys.DFS_USE_DFS_NETWORK_TOPOLOGY_KEY, true);
+ conf.setLong(DFSConfigKeys.DFS_BLOCK_SIZE_KEY, DEFAULT_BLOCK_SIZE);
+ conf.setInt(DFSConfigKeys.DFS_BYTES_PER_CHECKSUM_KEY,
+ DEFAULT_BLOCK_SIZE / 2);
+
+ if (cluster != null) {
+ cluster.shutdown();
+ }
+ cluster = new MiniDFSCluster.Builder(conf).numDataNodes(7).racks(racks)
+ .hosts(hosts).build();
+ cluster.waitActive();
+ nameNodeRpc = cluster.getNameNodeRpc();
+ namesystem = cluster.getNamesystem();
+ DistributedFileSystem fs = cluster.getFileSystem();
+ fs.enableErasureCodingPolicy("RS-3-2-1024k");
+ fs.setErasureCodingPolicy(new Path("/"), "RS-3-2-1024k");
+
+ final BlockManager bm = cluster.getNamesystem().getBlockManager();
+ final DatanodeManager dm = bm.getDatanodeManager();
+ assertTrue(dm.getNetworkTopology() instanceof DFSNetworkTopology);
+
+ String clientMachine = "/host4";
+ String clientRack = "/RACK4";
+ String src = "/test";
+
+ final DatanodeManager dnm = namesystem.getBlockManager().getDatanodeManager();
+ DatanodeDescriptor dnd4 = dnm.getDatanode(cluster.getDataNodes().get(4).getDatanodeId());
+ assertEquals(dnd4.getNetworkLocation(), clientRack);
+ dnm.getDatanodeAdminManager().startDecommission(dnd4);
+ short replication = 5;
+ short additionalReplication = 1;
+
+ try {
+ // Create the file with client machine
+ HdfsFileStatus fileStatus = namesystem.startFile(src, perm,
+ clientMachine, clientMachine, EnumSet.of(CreateFlag.CREATE), true,
+ replication, DEFAULT_BLOCK_SIZE * 1024 * 10, null, null, null, false);
+
+ //test chooseTarget for new file
+ LocatedBlock locatedBlock = nameNodeRpc.addBlock(src, clientMachine,
+ null, null, fileStatus.getFileId(), null, null);
+ HashMap<String, Integer> racksCount = new HashMap<String, Integer>();
+ doTestLocatedBlockRacks(racksCount, replication, 4, locatedBlock);
+
+ //test chooseTarget for existing file.
+ LocatedBlock additionalLocatedBlock =
+ nameNodeRpc.getAdditionalDatanode(src, fileStatus.getFileId(),
+ locatedBlock.getBlock(), locatedBlock.getLocations(),
+ locatedBlock.getStorageIDs(), DatanodeInfo.EMPTY_ARRAY,
+ additionalReplication, clientMachine);
+
+ racksCount.clear();
+ doTestLocatedBlockRacks(racksCount, additionalReplication + replication,
+ 4, additionalLocatedBlock);
+ assertEquals(racksCount.get("/RACK0"), (Integer)2);
+ assertEquals(racksCount.get("/RACK2"), (Integer)2);
+ } finally {
+ dnm.getDatanodeAdminManager().stopDecommission(dnd4);
+ }
+
+ //test if decommission successed
Review Comment:
```suggestion
//test if decommission succeeded
```
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestBlockPlacementPolicyRackFaultTolerant.java:
##########
@@ -167,6 +179,107 @@ private void doTestChooseTargetSpecialCase() throws Exception {
}
}
+ /**
+ * Verify decommission a dn which is an only node in its rack.
+ */
+ public void testPlacementWithOnlyOneNodeInRackDecommission() throws Exception {
+ Configuration conf = new HdfsConfiguration();
+ final String[] racks = {"/RACK0", "/RACK0", "/RACK2", "/RACK3", "/RACK4", "/RACK5", "/RACK2"};
+ final String[] hosts = {"/host0", "/host1", "/host2", "/host3", "/host4", "/host5", "/host6"};
+
+ // enables DFSNetworkTopology
+ conf.setClass(DFSConfigKeys.DFS_BLOCK_REPLICATOR_CLASSNAME_KEY,
+ BlockPlacementPolicyRackFaultTolerant.class,
+ BlockPlacementPolicy.class);
+ conf.setBoolean(DFSConfigKeys.DFS_USE_DFS_NETWORK_TOPOLOGY_KEY, true);
+ conf.setLong(DFSConfigKeys.DFS_BLOCK_SIZE_KEY, DEFAULT_BLOCK_SIZE);
+ conf.setInt(DFSConfigKeys.DFS_BYTES_PER_CHECKSUM_KEY,
+ DEFAULT_BLOCK_SIZE / 2);
+
+ if (cluster != null) {
+ cluster.shutdown();
+ }
+ cluster = new MiniDFSCluster.Builder(conf).numDataNodes(7).racks(racks)
+ .hosts(hosts).build();
+ cluster.waitActive();
+ nameNodeRpc = cluster.getNameNodeRpc();
+ namesystem = cluster.getNamesystem();
+ DistributedFileSystem fs = cluster.getFileSystem();
+ fs.enableErasureCodingPolicy("RS-3-2-1024k");
+ fs.setErasureCodingPolicy(new Path("/"), "RS-3-2-1024k");
+
+ final BlockManager bm = cluster.getNamesystem().getBlockManager();
+ final DatanodeManager dm = bm.getDatanodeManager();
+ assertTrue(dm.getNetworkTopology() instanceof DFSNetworkTopology);
+
+ String clientMachine = "/host4";
+ String clientRack = "/RACK4";
+ String src = "/test";
+
+ final DatanodeManager dnm = namesystem.getBlockManager().getDatanodeManager();
+ DatanodeDescriptor dnd4 = dnm.getDatanode(cluster.getDataNodes().get(4).getDatanodeId());
+ assertEquals(dnd4.getNetworkLocation(), clientRack);
+ dnm.getDatanodeAdminManager().startDecommission(dnd4);
+ short replication = 5;
+ short additionalReplication = 1;
+
+ try {
+ // Create the file with client machine
+ HdfsFileStatus fileStatus = namesystem.startFile(src, perm,
+ clientMachine, clientMachine, EnumSet.of(CreateFlag.CREATE), true,
+ replication, DEFAULT_BLOCK_SIZE * 1024 * 10, null, null, null, false);
+
+ //test chooseTarget for new file
+ LocatedBlock locatedBlock = nameNodeRpc.addBlock(src, clientMachine,
+ null, null, fileStatus.getFileId(), null, null);
+ HashMap<String, Integer> racksCount = new HashMap<String, Integer>();
+ doTestLocatedBlockRacks(racksCount, replication, 4, locatedBlock);
+
+ //test chooseTarget for existing file.
+ LocatedBlock additionalLocatedBlock =
+ nameNodeRpc.getAdditionalDatanode(src, fileStatus.getFileId(),
+ locatedBlock.getBlock(), locatedBlock.getLocations(),
+ locatedBlock.getStorageIDs(), DatanodeInfo.EMPTY_ARRAY,
+ additionalReplication, clientMachine);
+
+ racksCount.clear();
+ doTestLocatedBlockRacks(racksCount, additionalReplication + replication,
+ 4, additionalLocatedBlock);
+ assertEquals(racksCount.get("/RACK0"), (Integer)2);
+ assertEquals(racksCount.get("/RACK2"), (Integer)2);
+ } finally {
+ dnm.getDatanodeAdminManager().stopDecommission(dnd4);
+ }
+
+ //test if decommission successed
+ DatanodeDescriptor dnd3 = dnm.getDatanode(cluster.getDataNodes().get(3).getDatanodeId());
+ cluster.getNamesystem().writeLock();
+ try {
+ dm.getDatanodeAdminManager().startDecommission(dnd3);
+ } finally {
+ cluster.getNamesystem().writeUnlock();
+ }
+
+ // make sure the decommission finishes and the block in on 6 racks
Review Comment:
maybe
```suggestion
// make sure the decommission finishes and the block in on 4 racks
```
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetworkTopology.java:
##########
@@ -1015,4 +1024,108 @@ protected static boolean isNodeInScope(Node node, String scope) {
String nodeLocation = NodeBase.getPath(node) + NodeBase.PATH_SEPARATOR_STR;
return nodeLocation.startsWith(scope);
}
-}
\ No newline at end of file
+
+ /** @return the number of nonempty racks */
+ public int getNumOfNonEmptyRacks() {
+ return numOfRacks - numOfEmptyRacks;
+ }
+
+ /**
+ * Update empty rack number when add a node like recommision.
Review Comment:
```suggestion
* Update empty rack number when add a node like recommission.
```
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetworkTopology.java:
##########
@@ -1015,4 +1024,108 @@ protected static boolean isNodeInScope(Node node, String scope) {
String nodeLocation = NodeBase.getPath(node) + NodeBase.PATH_SEPARATOR_STR;
return nodeLocation.startsWith(scope);
}
-}
\ No newline at end of file
+
+ /** @return the number of nonempty racks */
+ public int getNumOfNonEmptyRacks() {
+ return numOfRacks - numOfEmptyRacks;
+ }
+
+ /**
+ * Update empty rack number when add a node like recommision.
+ * @param node node to be added; can be null
+ */
+ public void recommissionNode(Node node) {
+ if (node == null) {
+ return;
+ }
+ if (node instanceof InnerNode) {
+ throw new IllegalArgumentException(
+ "Not allow to remove an inner node: " + NodeBase.getPath(node));
+ }
+ netlock.writeLock().lock();
+ try {
+ decommissionNodes.remove(node.getName());
+ interAddNodeWithEmptyRack(node);
+ } finally {
+ netlock.writeLock().unlock();
+ }
+ }
+
+ /**
+ * Update empty rack number when remove a node like decommision.
Review Comment:
```suggestion
* Update empty rack number when remove a node like decommission.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org