ayushtkn commented on code in PR #6279:
URL: https://github.com/apache/hadoop/pull/6279#discussion_r1412752208
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicyExcludeSlowNodes.java:
##########
@@ -136,4 +139,40 @@ public void testChooseTargetExcludeSlowNodes() throws
Exception {
NameNode.LOG.info("Done working on it");
}
+ @Test
+ public void testSlowPeerTrackerEnabledClearSlowNodes() throws Exception {
+ namenode.getNamesystem().writeLock();
+ try {
+ // add nodes
+ for (DatanodeDescriptor dataNode : dataNodes) {
+ dnManager.addDatanode(dataNode);
+ }
+
+ // mock slow nodes
+ SlowPeerTracker tracker = dnManager.getSlowPeerTracker();
+ assert tracker != null;
Review Comment:
Use ``assertNotNull``
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/SlowPeerDisabledTracker.java:
##########
@@ -46,11 +46,6 @@ public class SlowPeerDisabledTracker extends SlowPeerTracker
{
public SlowPeerDisabledTracker(Configuration conf, Timer timer) {
super(conf, timer);
- final boolean dataNodePeerStatsEnabledVal =
- conf.getBoolean(DFSConfigKeys.DFS_DATANODE_PEER_STATS_ENABLED_KEY,
- DFSConfigKeys.DFS_DATANODE_PEER_STATS_ENABLED_DEFAULT);
- Preconditions.checkArgument(!dataNodePeerStatsEnabledVal,
- "SlowPeerDisabledTracker should only be used for disabled slow peer
stats.");
Review Comment:
Nice catch!!! This looks like a prod issue where reconfig won't work itself
I believe, you can't reconfig from ``true`` to ``false``
I was able to reproduce the issue by fixing the original test. I think we
should fix that test as well
```
diff --git
a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeReconfigure.java
b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeReconfigure.java
index 5a0f62a8117e..2bb1c124c90c 100644
---
a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeReconfigure.java
+++
b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeReconfigure.java
@@ -508,24 +508,21 @@ public void testSlowPeerTrackerEnabled() throws
Exception {
datanodeManager.getSlowPeerTracker().isSlowPeerTrackerEnabled());
assertTrue(datanodeManager.isSlowPeerCollectorInitialized());
- try {
- nameNode.reconfigurePropertyImpl(DFS_DATANODE_PEER_STATS_ENABLED_KEY,
"non-boolean");
- fail("should not reach here");
- } catch (ReconfigurationException e) {
- assertEquals(
- "Could not change property dfs.datanode.peer.stats.enabled from
'false' to 'non-boolean'",
- e.getMessage());
- }
+ LambdaTestUtils.intercept(ReconfigurationException.class,
+ "Could not change property dfs.datanode.peer.stats.enabled from
'false' to 'non-boolean'",
+ () -> nameNode.getConf().set(DFS_DATANODE_PEER_STATS_ENABLED_KEY,
+
nameNode.reconfigurePropertyImpl(DFS_DATANODE_PEER_STATS_ENABLED_KEY,
"non-boolean")));
- nameNode.reconfigurePropertyImpl(DFS_DATANODE_PEER_STATS_ENABLED_KEY,
"True");
+ nameNode.getConf().set(DFS_DATANODE_PEER_STATS_ENABLED_KEY,
+
nameNode.reconfigurePropertyImpl(DFS_DATANODE_PEER_STATS_ENABLED_KEY, "True"));
assertTrue("SlowNode tracker is still disabled. Reconfiguration could
not be successful",
datanodeManager.getSlowPeerTracker().isSlowPeerTrackerEnabled());
assertFalse(datanodeManager.isSlowPeerCollectorInitialized());
- nameNode.reconfigurePropertyImpl(DFS_DATANODE_PEER_STATS_ENABLED_KEY,
null);
+ nameNode.getConf().set(DFS_DATANODE_PEER_STATS_ENABLED_KEY,
+
nameNode.reconfigurePropertyImpl(DFS_DATANODE_PEER_STATS_ENABLED_KEY, null));
assertFalse("SlowNode tracker is still enabled. Reconfiguration could
not be successful",
datanodeManager.getSlowPeerTracker().isSlowPeerTrackerEnabled());
-
}
@Test
```
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicyExcludeSlowNodes.java:
##########
@@ -136,4 +139,40 @@ public void testChooseTargetExcludeSlowNodes() throws
Exception {
NameNode.LOG.info("Done working on it");
}
+ @Test
+ public void testSlowPeerTrackerEnabledClearSlowNodes() throws Exception {
+ namenode.getNamesystem().writeLock();
+ try {
+ // add nodes
+ for (DatanodeDescriptor dataNode : dataNodes) {
+ dnManager.addDatanode(dataNode);
+ }
+
+ // mock slow nodes
+ SlowPeerTracker tracker = dnManager.getSlowPeerTracker();
+ assert tracker != null;
+ OutlierMetrics outlierMetrics = new OutlierMetrics(0.0, 0.0, 0.0, 5.0);
+ tracker.addReport(dataNodes[0].getInfoAddr(), dataNodes[3].getInfoAddr(),
+ outlierMetrics);
+ tracker.addReport(dataNodes[1].getInfoAddr(), dataNodes[3].getInfoAddr(),
+ outlierMetrics);
+ tracker.addReport(dataNodes[2].getInfoAddr(), dataNodes[3].getInfoAddr(),
+ outlierMetrics);
+
+ // check slow nodes
+ assertFalse(dnManager.isSlowPeerCollectorInitialized());
+ GenericTestUtils.waitFor(
+ () -> DatanodeManager.getSlowNodesUuidSet().size() == 3, 100, 3000);
+ assertEquals(3, DatanodeManager.getSlowNodesUuidSet().size());
Review Comment:
no need of this, the above line does the same check
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
