You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/08/27 10:44:00 UTC
[GitHub] [ozone] sodonnel opened a new pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
sodonnel opened a new pull request #2591:
URL: https://github.com/apache/ozone/pull/2591
## What changes were proposed in this pull request?
TestContainerReplication#testSkipDecommissionAndMaintenanceNode() was added as part of HDDS-5296. The test runs 3 times and sleeps for 30 seconds on each run to wait to ensure replication never hits an out of service node. Aside from the 30 second sleep, each run of the test takes about 1 minute, so 3 minutes in total for this test.
This class takes about 370 seconds on every pull request:
```
[INFO] Running org.apache.hadoop.ozone.container.TestContainerReplication
[INFO] Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 368.92 s - in org.apache.hadoop.ozone.container.TestContainerReplication
```
I feel this integration test is not needed. The logic we are testing here, is actually part of the placement policy and we can test for the original defect in TestSCMContainerPlacementRackAware much more efficiently and reliably.
Here I have added a new test to TestSCMContainerPlacementRackAware and removed the test mentioned above.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5690
## How was this patch tested?
New and existing tests
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#issuecomment-907971529
+1, LGTM.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#discussion_r697422649
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/TestSCMContainerPlacementRackAware.java
##########
@@ -524,4 +527,33 @@ public void testvalidateContainerPlacementSingleRackCluster() {
assertTrue(stat.isPolicySatisfied());
assertEquals(0, stat.misReplicationCount());
}
+
+ @Test
+ public void testOutOfServiceNodesNotSelected() throws SCMException {
+ List<DatanodeDetails> datanodeDetails =
+ policy.chooseDatanodes(null, null, 1, 0, 0);
+ Assert.assertEquals(1, datanodeDetails.size());
Review comment:
I guess those lines are not really needed. My intention was to show we can get a node when all are in service, but then only get an IN_SERVICE node (or exception) if only 1 node is IN_SERVICE. However those lines are not very useful at all. I will remove them.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#discussion_r697880904
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/TestSCMContainerPlacementRackAware.java
##########
@@ -524,4 +527,28 @@ public void testvalidateContainerPlacementSingleRackCluster() {
assertTrue(stat.isPolicySatisfied());
assertEquals(0, stat.misReplicationCount());
}
+
+ @Test
+ public void testOutOfServiceNodesNotSelected() {
+ // Set all the nodes to out of service
+ for (DatanodeInfo dn : dnInfos) {
+ dn.setNodeStatus(new NodeStatus(DECOMMISSIONED, HEALTHY));
+ }
+
+ for (int i=0; i<10; i++) {
+ // Set a random DN to in_service and ensure it is always picked
+ int index = new Random().nextInt(dnInfos.size());
+ dnInfos.get(index).setNodeStatus(NodeStatus.inServiceHealthy());
+ try {
+ List<DatanodeDetails> datanodeDetails =
+ policy.chooseDatanodes(null, null, 1, 0, 0);
+ Assert.assertEquals(dnInfos.get(index), datanodeDetails.get(0));
+ } catch (SCMException e) {
+ // If we get SCMException: No satisfied datanode to meet the ... this is
Review comment:
Should we add a check at the end to make sure that `policy.chooseDatanodes` worked at least once? Or retry the test until it worked once.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#discussion_r697880904
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/TestSCMContainerPlacementRackAware.java
##########
@@ -524,4 +527,28 @@ public void testvalidateContainerPlacementSingleRackCluster() {
assertTrue(stat.isPolicySatisfied());
assertEquals(0, stat.misReplicationCount());
}
+
+ @Test
+ public void testOutOfServiceNodesNotSelected() {
+ // Set all the nodes to out of service
+ for (DatanodeInfo dn : dnInfos) {
+ dn.setNodeStatus(new NodeStatus(DECOMMISSIONED, HEALTHY));
+ }
+
+ for (int i=0; i<10; i++) {
+ // Set a random DN to in_service and ensure it is always picked
+ int index = new Random().nextInt(dnInfos.size());
+ dnInfos.get(index).setNodeStatus(NodeStatus.inServiceHealthy());
+ try {
+ List<DatanodeDetails> datanodeDetails =
+ policy.chooseDatanodes(null, null, 1, 0, 0);
+ Assert.assertEquals(dnInfos.get(index), datanodeDetails.get(0));
+ } catch (SCMException e) {
+ // If we get SCMException: No satisfied datanode to meet the ... this is
Review comment:
Should we add a check at the end to make sure that `policy.chooseDatanodes` worked at least once?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#issuecomment-907199782
With this change, the new runtime is:
```
[INFO] Running org.apache.hadoop.ozone.container.TestContainerReplication
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 150.429 s - in org.apache.hadoop.ozone.container.TestContainerReplication
```
Saving about 170 seconds per run.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#issuecomment-909093606
@kerneltime Have you any further concerns? We have a few +1's here, so I will commit this tomorrow unless you would like any further changes?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#discussion_r697403608
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/TestSCMContainerPlacementRackAware.java
##########
@@ -524,4 +527,33 @@ public void testvalidateContainerPlacementSingleRackCluster() {
assertTrue(stat.isPolicySatisfied());
assertEquals(0, stat.misReplicationCount());
}
+
+ @Test
+ public void testOutOfServiceNodesNotSelected() throws SCMException {
+ List<DatanodeDetails> datanodeDetails =
+ policy.chooseDatanodes(null, null, 1, 0, 0);
+ Assert.assertEquals(1, datanodeDetails.size());
Review comment:
Not sure these 3 lines are helpful. Would you give a little more explanation?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#discussion_r697977474
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/TestSCMContainerPlacementRackAware.java
##########
@@ -524,4 +527,28 @@ public void testvalidateContainerPlacementSingleRackCluster() {
assertTrue(stat.isPolicySatisfied());
assertEquals(0, stat.misReplicationCount());
}
+
+ @Test
+ public void testOutOfServiceNodesNotSelected() {
+ // Set all the nodes to out of service
+ for (DatanodeInfo dn : dnInfos) {
+ dn.setNodeStatus(new NodeStatus(DECOMMISSIONED, HEALTHY));
+ }
+
+ for (int i=0; i<10; i++) {
+ // Set a random DN to in_service and ensure it is always picked
+ int index = new Random().nextInt(dnInfos.size());
+ dnInfos.get(index).setNodeStatus(NodeStatus.inServiceHealthy());
+ try {
+ List<DatanodeDetails> datanodeDetails =
+ policy.chooseDatanodes(null, null, 1, 0, 0);
+ Assert.assertEquals(dnInfos.get(index), datanodeDetails.get(0));
+ } catch (SCMException e) {
+ // If we get SCMException: No satisfied datanode to meet the ... this is
Review comment:
The retry count in the provider is hard coded at the moment, and this is not a realistic real-world scenario. If there are only a few healthy nodes out of 100's in a cluster, then the cluster has much bigger problems. If we want to change the behaviour of the provider, then we should discuss that in a new Jira.
The contact of the provider is to provide the number of requested nodes, or throw the SCM Exception. So either we get the single IN_SERVICE node, or we get the exception. The key is that we never get the out_of_service node, which is the defect the original change was fixing. If you removed the fix for the original issue, this test would always fail as it is most likely to find the out_of_service node each time, and over 10 tries in the loop it would almost certainly find an out_of_service node and fail.
I cannot think of a better way to test this without changing the provider code (which I don't want to do in this Jira), but if you have a better idea that will never be flaky I am happy to use it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#discussion_r698129289
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/TestSCMContainerPlacementRackAware.java
##########
@@ -524,4 +527,28 @@ public void testvalidateContainerPlacementSingleRackCluster() {
assertTrue(stat.isPolicySatisfied());
assertEquals(0, stat.misReplicationCount());
}
+
+ @Test
+ public void testOutOfServiceNodesNotSelected() {
+ // Set all the nodes to out of service
+ for (DatanodeInfo dn : dnInfos) {
+ dn.setNodeStatus(new NodeStatus(DECOMMISSIONED, HEALTHY));
+ }
+
+ for (int i=0; i<10; i++) {
+ // Set a random DN to in_service and ensure it is always picked
+ int index = new Random().nextInt(dnInfos.size());
+ dnInfos.get(index).setNodeStatus(NodeStatus.inServiceHealthy());
+ try {
+ List<DatanodeDetails> datanodeDetails =
+ policy.chooseDatanodes(null, null, 1, 0, 0);
+ Assert.assertEquals(dnInfos.get(index), datanodeDetails.get(0));
+ } catch (SCMException e) {
+ // If we get SCMException: No satisfied datanode to meet the ... this is
Review comment:
The networkTopology doesn't have the node healthy information currently. There is chance that chooseDatanodes will throw exception after 3 internal retries that it cannot find a proper node that is healthy if there are two many unhealthy nodes in the cluster. One improvement I can think of is add the the healthy status and storage space full status into the NetworkTopology. And I agree with @sodonnel , we can discuss it in another JIRA.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#discussion_r697974871
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/TestSCMContainerPlacementRackAware.java
##########
@@ -524,4 +527,28 @@ public void testvalidateContainerPlacementSingleRackCluster() {
assertTrue(stat.isPolicySatisfied());
assertEquals(0, stat.misReplicationCount());
}
+
+ @Test
+ public void testOutOfServiceNodesNotSelected() {
+ // Set all the nodes to out of service
+ for (DatanodeInfo dn : dnInfos) {
+ dn.setNodeStatus(new NodeStatus(DECOMMISSIONED, HEALTHY));
+ }
+
+ for (int i=0; i<10; i++) {
+ // Set a random DN to in_service and ensure it is always picked
+ int index = new Random().nextInt(dnInfos.size());
+ dnInfos.get(index).setNodeStatus(NodeStatus.inServiceHealthy());
+ try {
+ List<DatanodeDetails> datanodeDetails =
+ policy.chooseDatanodes(null, null, 1, 0, 0);
+ Assert.assertEquals(dnInfos.get(index), datanodeDetails.get(0));
+ } catch (SCMException e) {
+ // If we get SCMException: No satisfied datanode to meet the ... this is
Review comment:
Should we then change the setup and retry count to make sure this test actually tests as intended. A successful run here would not imply that the only node selected is the one that is healthy.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#issuecomment-909093606
@kerneltime Have you any further concerns? We have a few +1's here, so I will commit this tomorrow unless you would like any further changes?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#discussion_r697889335
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/TestSCMContainerPlacementRackAware.java
##########
@@ -524,4 +527,28 @@ public void testvalidateContainerPlacementSingleRackCluster() {
assertTrue(stat.isPolicySatisfied());
assertEquals(0, stat.misReplicationCount());
}
+
+ @Test
+ public void testOutOfServiceNodesNotSelected() {
+ // Set all the nodes to out of service
+ for (DatanodeInfo dn : dnInfos) {
+ dn.setNodeStatus(new NodeStatus(DECOMMISSIONED, HEALTHY));
+ }
+
+ for (int i=0; i<10; i++) {
+ // Set a random DN to in_service and ensure it is always picked
+ int index = new Random().nextInt(dnInfos.size());
+ dnInfos.get(index).setNodeStatus(NodeStatus.inServiceHealthy());
+ try {
+ List<DatanodeDetails> datanodeDetails =
+ policy.chooseDatanodes(null, null, 1, 0, 0);
+ Assert.assertEquals(dnInfos.get(index), datanodeDetails.get(0));
+ } catch (SCMException e) {
+ // If we get SCMException: No satisfied datanode to meet the ... this is
Review comment:
I think that would risk making the test flaky. When there are a large number of datanodes (say 15) and we put all out of service but 1, there is a reasonably high chance it will never find a node to use in 10 tries. I think it only retries 3 times inside the policy so it has a 1 in 15 chance on the first try, and 1 in 14 on the second try and 1 in 13 on the 3rd. Its actually more likely to fail than find a node.
I could argue that is a problem with the provider, but that logic was put in to avoid infinite loops, and we are not trying to fix the provider here. Probably the provider should keep trying until all nodes have been tried, but that would be a topic for another Jira I think.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel merged pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
sodonnel merged pull request #2591:
URL: https://github.com/apache/ozone/pull/2591
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #2591: HDDS-5690. Speed up TestContainerReplication by removing testSkipDemmissionAndMaintenanceNode
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #2591:
URL: https://github.com/apache/ozone/pull/2591#issuecomment-909093606
@kerneltime Have you any further concerns? We have a few +1's here, so I will commit this tomorrow unless you would like any further changes?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org