pan3793 commented on code in PR #8910:
URL: https://github.com/apache/hadoop/pull/8910#discussion_r4464747711
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockPlacementPolicyRackFaultTolerant.java:
##########
@@ -195,6 +195,10 @@ private void chooseEvenlyFromRemainingRacks(Node writer,
// To improve performance, the maximum value of
'bestEffortMaxNodesPerRack'
// is calculated only when it is not possible to select a node.
if (numResultsOflastChoose == results.size()) {
+ if (results.isEmpty()) {
Review Comment:
With this fix, an ALL_SSD write on a cluster with no SSD succeeds through
the storage fallback, but each block allocation still logs the WARN at line 145
of this file. It blames rack configuration when the cause is storage type.
`results` is empty there only when no node could be chosen, so the rack limit
is not involved. Please log that at WARN only when `results` is non-empty.
```java
if (!results.isEmpty()) {
LOG.warn(...);
}
```
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestBlockPlacementPolicyRackFaultTolerant.java:
##########
@@ -280,6 +282,32 @@ public Boolean get() {
}
}
+ /**
+ * Verify that ALL_SSD falls back to DISK when there is no SSD. Before the
+ * fix, choosing targets threw a NoSuchElementException once no node could
+ * be chosen at all, which bypassed the storage type fallback in
+ * BlockPlacementPolicyDefault and failed the write.
+ */
+ @Test
+ public void testChooseTargetFallbackStorageType() throws Exception {
Review Comment:
This covers one path: a replicated file with 3 replicas on 10 racks. Please
also cover:
1. An EC file with ALL_SSD, since EC is this policy's default user.
2. Replication 11 on 10 racks, which goes through the two-pass branch of
`chooseTargetInOrder`.
3. The stale-node retry, which the PR description says works again.
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestBlockPlacementPolicyRackFaultTolerant.java:
##########
@@ -280,6 +282,32 @@ public Boolean get() {
}
}
+ /**
+ * Verify that ALL_SSD falls back to DISK when there is no SSD. Before the
Review Comment:
Please drop the "Before the fix..." history, which belongs in the JIRA.
"Verify that ALL_SSD falls back to DISK when there is no SSD." is enough.
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestBlockPlacementPolicyRackFaultTolerant.java:
##########
@@ -280,6 +282,32 @@ public Boolean get() {
}
}
+ /**
+ * Verify that ALL_SSD falls back to DISK when there is no SSD. Before the
+ * fix, choosing targets threw a NoSuchElementException once no node could
+ * be chosen at all, which bypassed the storage type fallback in
+ * BlockPlacementPolicyDefault and failed the write.
+ */
+ @Test
+ public void testChooseTargetFallbackStorageType() throws Exception {
+ DistributedFileSystem fs = cluster.getFileSystem();
+ Path dir = new Path("/all_ssd");
+ fs.mkdirs(dir);
+ fs.setStoragePolicy(dir, "ALL_SSD");
+ Path file = new Path(dir, "file");
+ DFSTestUtil.createFile(fs, file, DEFAULT_BLOCK_SIZE, (short) 3, 0L);
+ DFSTestUtil.waitReplication(fs, file, (short) 3);
+
+ LocatedBlocks locatedBlocks =
+ fs.getClient().getLocatedBlocks(file.toString(), 0);
+ for (LocatedBlock block : locatedBlocks.getLocatedBlocks()) {
Review Comment:
All assertions are inside this loop, so an empty block list passes. Please
assert the block count first:
```java
assertEquals(1, locatedBlocks.locatedBlockCount());
```
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestBlockPlacementPolicyRackFaultTolerant.java:
##########
@@ -280,6 +282,32 @@ public Boolean get() {
}
}
+ /**
+ * Verify that ALL_SSD falls back to DISK when there is no SSD. Before the
+ * fix, choosing targets threw a NoSuchElementException once no node could
+ * be chosen at all, which bypassed the storage type fallback in
+ * BlockPlacementPolicyDefault and failed the write.
+ */
+ @Test
+ public void testChooseTargetFallbackStorageType() throws Exception {
+ DistributedFileSystem fs = cluster.getFileSystem();
+ Path dir = new Path("/all_ssd");
+ fs.mkdirs(dir);
+ fs.setStoragePolicy(dir, "ALL_SSD");
+ Path file = new Path(dir, "file");
+ DFSTestUtil.createFile(fs, file, DEFAULT_BLOCK_SIZE, (short) 3, 0L);
+ DFSTestUtil.waitReplication(fs, file, (short) 3);
+
+ LocatedBlocks locatedBlocks =
+ fs.getClient().getLocatedBlocks(file.toString(), 0);
+ for (LocatedBlock block : locatedBlocks.getLocatedBlocks()) {
+ assertEquals(3, block.getStorageTypes().length);
Review Comment:
This doesn't check rack placement, so three replicas on one rack would pass.
`doTestLocatedBlock` doesn't catch that either, since it only counts racks
holding a replica. With 3 replicas on 10 racks, please assert distinct racks:
```java
Set<String> racks = new HashSet<>();
for (DatanodeInfo node : block.getLocations()) {
racks.add(node.getNetworkLocation());
}
assertEquals(3, racks.size());
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
