You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Tejaskriya (via GitHub)" <gi...@apache.org> on 2024/03/01 08:31:39 UTC
[PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Tejaskriya opened a new pull request, #6305:
URL: https://github.com/apache/ozone/pull/6305
## What changes were proposed in this pull request?
The FindSourceGreedy class maintains a priority queue of source datanodes. This class is used to get the next source DN for balancing. According to current design, a datanode can be involved in multiple source->target pairings in one iteration of container balancer.
When FindSourceGreedy#getNextCandidateSourceDataNode() is called during an iteration, it polls a DN from the queue. This DN is re queued when FindSourceGreedy#increaseSizeLeaving is called. Problem is that if ContainerBalancer#moveContainer returns false, then increaseSizeLeaving will not be called and the DN doesn't get added back.
If the failure of move operation is due to some errors related to the container selected in that run, then we should be adding back the DN to the queue. That way in the next run, the source DN could still be considered.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7252
## How was this patch tested?
--pending
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1513989294
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -856,19 +860,34 @@ private boolean moveContainer(DatanodeDetails source,
" {} failed: {}",
moveSelection.getContainerID(), source.getUuidString(),
moveSelection.getTargetNode().getUuidString(), result);
+ // add source back if move failed due to container related errors
+ if (result == MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_EXIST_IN_TARGET ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_CONTAINER_NOT_CLOSED ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_DELETION ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_REPLICATION) {
+ findSourceStrategy.addBackSourceDataNode(source);
+ }
Review Comment:
We don't need to add the source back here I think because it'd already have been added back in
```
if (moveContainer(source, moveSelection)) {
// consider move successful for now, and update selection criteria
updateTargetsAndSelectionCriteria(moveSelection, source);
}
```
when `moveContainer` returns true.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #6305:
URL: https://github.com/apache/ozone/pull/6305#issuecomment-2019786522
@siddhantsangwan Thank you for finding the hack to test it! It works, I have added it in the unit tests.
@siddhantsangwan @symious Could you please review this updated PR? I have incorporated the suggested 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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on PR #6305:
URL: https://github.com/apache/ozone/pull/6305#issuecomment-2046609475
Holding off on merging while we're still discussing a point above.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1553198489
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -563,6 +566,9 @@ private boolean processMoveSelection(DatanodeDetails source,
} catch (ContainerNotFoundException e) {
LOG.warn("Could not get container {} from Container Manager before " +
"starting a container move", containerID, e);
+ // add source back to queue as a different container can be selected in next run.
+ findSourceStrategy.addBackSourceDataNode(source);
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
Review Comment:
> I think we need reconsider this container exclusion to be at source level only.
What's your reasoning behind this? If container manager can't find this container I think we should avoid this container for any DN.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1567392905
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -862,13 +870,24 @@ private boolean moveContainer(DatanodeDetails source,
} catch (ContainerNotFoundException e) {
LOG.warn("Could not find Container {} for container move",
containerID, e);
+ // add source back to queue as a different container can be selected in next run.
+ findSourceStrategy.addBackSourceDataNode(source);
+ // exclude the container which caused failure of move to avoid error in next run.
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
metrics.incrementNumContainerMovesFailedInLatestIteration(1);
return false;
- } catch (NodeNotFoundException | TimeoutException |
- ContainerReplicaNotFoundException e) {
+ } catch (NodeNotFoundException | TimeoutException e) {
LOG.warn("Container move failed for container {}", containerID, e);
metrics.incrementNumContainerMovesFailedInLatestIteration(1);
return false;
+ } catch (ContainerReplicaNotFoundException e) {
+ LOG.warn("Container move failed for container {}", containerID, e);
+ metrics.incrementNumContainerMovesFailedInLatestIteration(1);
+ // add source back to queue for replica not found only
+ findSourceStrategy.addBackSourceDataNode(source);
+ // exclude the container whose replica caused failure of move to avoid error in next run.
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
Review Comment:
I have updated the PR. Thank you for the review! Could you please approve the workflows and merge it if everything looks fine?
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1558898986
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -563,6 +566,9 @@ private boolean processMoveSelection(DatanodeDetails source,
} catch (ContainerNotFoundException e) {
LOG.warn("Could not get container {} from Container Manager before " +
"starting a container move", containerID, e);
+ // add source back to queue as a different container can be selected in next run.
+ findSourceStrategy.addBackSourceDataNode(source);
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
Review Comment:
When a container is not found in the SCM, the problem is likely outside of balancer and will affect any other sources trying to move the same container later. So I think we should exclude the container for all sources when it's a `ContainerInfo` related error like in the above case.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan merged PR #6305:
URL: https://github.com/apache/ozone/pull/6305
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1522691055
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -856,19 +860,34 @@ private boolean moveContainer(DatanodeDetails source,
" {} failed: {}",
moveSelection.getContainerID(), source.getUuidString(),
moveSelection.getTargetNode().getUuidString(), result);
+ // add source back if move failed due to container related errors
+ if (result == MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_EXIST_IN_TARGET ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_CONTAINER_NOT_CLOSED ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_DELETION ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_REPLICATION) {
+ findSourceStrategy.addBackSourceDataNode(source);
+ }
Review Comment:
We need to handle the case where the scheduled move completes with a `MoveResult` other than `COMPLETED`, before the if...else code of `if (future.isDone())` gets executed. In this case, this line `return result == MoveManager.MoveResult.COMPLETED;` will evaluate to false and the source datanode will not get added back. However, instead of handling this case in the whenComplete block, I suggest handling it this way:
```
if (future.isDone()) {
if (future.isCompletedExceptionally()) {
return false;
} else {
MoveManager.MoveResult result = future.join();
moveSelectionToFutureMap.put(moveSelection, future);
if (result == MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE ||
result == MoveManager.MoveResult.REPLICATION_FAIL_EXIST_IN_TARGET ||
result == MoveManager.MoveResult.REPLICATION_FAIL_CONTAINER_NOT_CLOSED ||
result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_DELETION ||
result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_REPLICATION) {
findSourceStrategy.addBackSourceDataNode(source);
}
return result == MoveManager.MoveResult.COMPLETED;
}
} else {
moveSelectionToFutureMap.put(moveSelection, future);
return true;
}
```
This will achieve the same result and I think is a better way mainly because of thread safety issues. Any code in the `whenComplete` block can be called from a thread other than the balancing thread. This means we need to care about thread safety problems, such as concurrent accesses to the priority queue that `findSourceStrategy.addBackSourceDataNode(source)` is changing. We're better off avoiding this to prevent multithreading issues.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1550064165
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -563,6 +566,9 @@ private boolean processMoveSelection(DatanodeDetails source,
} catch (ContainerNotFoundException e) {
LOG.warn("Could not get container {} from Container Manager before " +
"starting a container move", containerID, e);
+ // add source back to queue as a different container can be selected in next run.
+ findSourceStrategy.addBackSourceDataNode(source);
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
Review Comment:
How about same container in another source? do that also needs keep excluding? I think we need reconsider this container exclusion to be at source level only.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1552956831
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -563,6 +566,9 @@ private boolean processMoveSelection(DatanodeDetails source,
} catch (ContainerNotFoundException e) {
LOG.warn("Could not get container {} from Container Manager before " +
"starting a container move", containerID, e);
+ // add source back to queue as a different container can be selected in next run.
+ findSourceStrategy.addBackSourceDataNode(source);
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
Review Comment:
Thanks for the review! I have created a Map of Dn and containers to be excluded for that DN. Could you please take a look at the recent changes?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -563,6 +566,9 @@ private boolean processMoveSelection(DatanodeDetails source,
} catch (ContainerNotFoundException e) {
LOG.warn("Could not get container {} from Container Manager before " +
"starting a container move", containerID, e);
+ // add source back to queue as a different container can be selected in next run.
+ findSourceStrategy.addBackSourceDataNode(source);
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
Review Comment:
Thanks for the review! I have created a Map of DN and containers to be excluded for that DN. Could you please take a look at the recent 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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "symious (via GitHub)" <gi...@apache.org>.
symious commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1515370908
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -856,19 +860,34 @@ private boolean moveContainer(DatanodeDetails source,
" {} failed: {}",
moveSelection.getContainerID(), source.getUuidString(),
moveSelection.getTargetNode().getUuidString(), result);
+ // add source back if move failed due to container related errors
+ if (result == MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_EXIST_IN_TARGET ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_CONTAINER_NOT_CLOSED ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_DELETION ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_REPLICATION) {
+ findSourceStrategy.addBackSourceDataNode(source);
+ }
Review Comment:
Since the `whenComplete` is process after the move work, which is async, I'm not sure if this will help with the moveSelection, the selection part might have finished already.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1513989294
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -856,19 +860,34 @@ private boolean moveContainer(DatanodeDetails source,
" {} failed: {}",
moveSelection.getContainerID(), source.getUuidString(),
moveSelection.getTargetNode().getUuidString(), result);
+ // add source back if move failed due to container related errors
+ if (result == MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_EXIST_IN_TARGET ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_CONTAINER_NOT_CLOSED ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_DELETION ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_REPLICATION) {
+ findSourceStrategy.addBackSourceDataNode(source);
+ }
Review Comment:
We don't need to the source back here I think because it'd already have been added back in
```
if (moveContainer(source, moveSelection)) {
// consider move successful for now, and update selection criteria
updateTargetsAndSelectionCriteria(moveSelection, source);
}
```
when `moveContainer` returns true.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1522726768
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindSourceGreedy.java:
##########
@@ -138,6 +138,11 @@ public void removeCandidateSourceDataNode(DatanodeDetails dui) {
potentialSources.removeIf(a -> a.getDatanodeDetails().equals(dui));
}
+ @Override
+ public void addBackSourceDataNode(DatanodeDetails dui) {
+ potentialSources.add(nodeManager.getUsageInfo(dui));
+ }
Review Comment:
A caller could use this method to a Datanode that wasn't previously removed from the priority queue. This could result in multiple instances of the same Datanode in the queue. Let's add a javadoc to the interface and explicitly call out that anyone using this method should ensure the Datanode was previously removed or that it isn't present in the queue.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on PR #6305:
URL: https://github.com/apache/ozone/pull/6305#issuecomment-1993801520
@Tejaskriya We were discussing how to test this change earlier. I came up with a bit of a hack to test it.
```
@Test
public void testSourceDatanodeAddedBack()
throws NodeNotFoundException, IOException, IllegalContainerBalancerStateException,
InvalidContainerBalancerConfigurationException, TimeoutException, InterruptedException {
when(moveManager.move(any(ContainerID.class),
any(DatanodeDetails.class),
any(DatanodeDetails.class)))
.thenReturn(CompletableFuture.completedFuture(MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE))
.thenReturn(CompletableFuture.completedFuture(MoveManager.MoveResult.COMPLETED));
balancerConfiguration.setThreshold(10);
balancerConfiguration.setIterations(1);
balancerConfiguration.setMaxSizeEnteringTarget(10 * STORAGE_UNIT);
balancerConfiguration.setMaxSizeToMovePerIteration(100 * STORAGE_UNIT);
balancerConfiguration.setMaxDatanodesPercentageToInvolvePerIteration(100);
String includeNodes = nodesInCluster.get(0).getDatanodeDetails().getHostName() + "," +
nodesInCluster.get(nodesInCluster.size() - 1).getDatanodeDetails().getHostName();
balancerConfiguration.setIncludeNodes(includeNodes);
startBalancer(balancerConfiguration);
GenericTestUtils.waitFor(() -> ContainerBalancerTask.IterationResult.ITERATION_COMPLETED ==
containerBalancerTask.getIterationResult(), 10, 50);
assertEquals(2, containerBalancerTask.getCountDatanodesInvolvedPerIteration());
assertTrue(containerBalancerTask.getMetrics().getNumContainerMovesCompletedInLatestIteration() >= 1);
assertThat(containerBalancerTask.getMetrics().getNumContainerMovesFailed()).isEqualTo(1);
stopBalancer();
}
```
It uses the includeNodes configuration to ensure only one under and one over utilized node is involved in balancing. This way we're able to test whether the same source was or wasn't added back. What do you 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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1521054962
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -856,19 +860,34 @@ private boolean moveContainer(DatanodeDetails source,
" {} failed: {}",
moveSelection.getContainerID(), source.getUuidString(),
moveSelection.getTargetNode().getUuidString(), result);
+ // add source back if move failed due to container related errors
+ if (result == MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_EXIST_IN_TARGET ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_CONTAINER_NOT_CLOSED ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_DELETION ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_REPLICATION) {
+ findSourceStrategy.addBackSourceDataNode(source);
+ }
Review Comment:
Actually, I'm rethinking my comment. We might need to add back the source in the `whenComplete` logic as well. I'll get back here once I've understood what we can do.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1547327496
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerTask.java:
##########
@@ -1114,6 +1114,35 @@ public void balancerShouldExcludeECContainersWhenLegacyRmIsEnabled()
}
}
+ @Test
Review Comment:
Let's add java doc and comments explaining what this test is doing as it may not be obvious to someone else.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindSourceGreedy.java:
##########
@@ -138,6 +138,14 @@ public void removeCandidateSourceDataNode(DatanodeDetails dui) {
potentialSources.removeIf(a -> a.getDatanodeDetails().equals(dui));
}
+ @Override
+ public void addBackSourceDataNode(DatanodeDetails dn) {
+ DatanodeUsageInfo dui = nodeManager.getUsageInfo(dn);
+ if (!potentialSources.contains(dui)) {
Review Comment:
We don't want to pay the performance cost of `contains` for a priority queue every time we're adding back a source. Let's avoid this check.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindSourceStrategy.java:
##########
@@ -45,6 +45,16 @@ public interface FindSourceStrategy {
*/
void removeCandidateSourceDataNode(DatanodeDetails dui);
+ /**
+ * add the specified data node to the candidate source
+ * data nodes.
+ * Caller of this method must ensure that Datanode is
+ * not currently a candidate source
Review Comment:
```suggestion
* This method does not check whether the specified Datanode is already present in the Collection. Callers must take the responsibility of checking and removing the Datanode before adding, if required.
```
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerTask.java:
##########
@@ -1114,6 +1114,35 @@ public void balancerShouldExcludeECContainersWhenLegacyRmIsEnabled()
}
}
+ @Test
+ public void testSourceDatanodeAddedBack()
+ throws NodeNotFoundException, IOException, IllegalContainerBalancerStateException,
+ InvalidContainerBalancerConfigurationException, TimeoutException, InterruptedException {
+
+ when(moveManager.move(any(ContainerID.class),
+ any(DatanodeDetails.class),
+ any(DatanodeDetails.class)))
+ .thenReturn(CompletableFuture.completedFuture(MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE))
+ .thenReturn(CompletableFuture.completedFuture(MoveManager.MoveResult.COMPLETED));
+ balancerConfiguration.setThreshold(10);
+ balancerConfiguration.setIterations(1);
+ balancerConfiguration.setMaxSizeEnteringTarget(10 * STORAGE_UNIT);
+ balancerConfiguration.setMaxSizeToMovePerIteration(100 * STORAGE_UNIT);
+ balancerConfiguration.setMaxDatanodesPercentageToInvolvePerIteration(100);
+ String includeNodes = nodesInCluster.get(0).getDatanodeDetails().getHostName() + "," +
+ nodesInCluster.get(nodesInCluster.size() - 1).getDatanodeDetails().getHostName();
+ balancerConfiguration.setIncludeNodes(includeNodes);
+
+ startBalancer(balancerConfiguration);
+ GenericTestUtils.waitFor(() -> ContainerBalancerTask.IterationResult.ITERATION_COMPLETED ==
+ containerBalancerTask.getIterationResult(), 10, 50);
+
+ assertEquals(2, containerBalancerTask.getCountDatanodesInvolvedPerIteration());
Review Comment:
Let's also assert that the two DNs involved are the ones we expect (specified in includeNodes).
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -881,6 +896,14 @@ private boolean moveContainer(DatanodeDetails source,
} else {
MoveManager.MoveResult result = future.join();
moveSelectionToFutureMap.put(moveSelection, future);
+ if (result == MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_EXIST_IN_TARGET ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_CONTAINER_NOT_CLOSED ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_DELETION ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_REPLICATION) {
+ findSourceStrategy.addBackSourceDataNode(source);
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
Review Comment:
We don't need to exclude the container here I think. These seem to be replica specific and intermittent failures.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1558898986
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -563,6 +566,9 @@ private boolean processMoveSelection(DatanodeDetails source,
} catch (ContainerNotFoundException e) {
LOG.warn("Could not get container {} from Container Manager before " +
"starting a container move", containerID, e);
+ // add source back to queue as a different container can be selected in next run.
+ findSourceStrategy.addBackSourceDataNode(source);
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
Review Comment:
When a container is not found in the SCM, the problem is likely outside of balancer and will affect any other sources trying to move the same container later. So I think we should exclude the container when it's a `ContainerInfo` related error like in the above case.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1519227463
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -856,19 +860,34 @@ private boolean moveContainer(DatanodeDetails source,
" {} failed: {}",
moveSelection.getContainerID(), source.getUuidString(),
moveSelection.getTargetNode().getUuidString(), result);
+ // add source back if move failed due to container related errors
+ if (result == MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_EXIST_IN_TARGET ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_CONTAINER_NOT_CLOSED ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_DELETION ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_REPLICATION) {
+ findSourceStrategy.addBackSourceDataNode(source);
+ }
Review Comment:
In my understanding, in case the move is completed within one iteration of the containerBalancer, then in the next run of the infinite loop wouldn't this change would help by adding the source back? What I mean is:
```
ContainerBalancer # doIteration() {
... //this logic runs for each iteration
while(true) {
...
// in one run of this loop, the source is selected and move is scheduled
// in some consecutive run, the previous move is completed with false result,
// so following current PR changes, the source is added back.
// so now it can be considered for source selection next time
...
}
...
}
```
If we see in doIteration -> processMoveSelection -> updateTargetsAndSelectionCriteria -> the same is done if move was successful.
This change simply does the same if move is failing too.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #6305:
URL: https://github.com/apache/ozone/pull/6305#issuecomment-2032006655
@siddhantsangwan Thank you for the review! I have made the changes suggested by you and addressed the remaining comments with appropriate code changes. Could you please take another look at the PR now?
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1552864793
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java:
##########
@@ -68,6 +69,7 @@ public ContainerBalancerSelectionCriteria(
this.replicationManager = replicationManager;
this.containerManager = containerManager;
selectedContainers = new HashSet<>();
+ excludeContainersDueToFailure = new HashSet<>();
Review Comment:
In ContainerBalancerTask#initializeIteration, in the end we are creating a new object for containerBalancerSelectionCriteria
```
selectionCriteria = new ContainerBalancerSelectionCriteria(config,
nodeManager, replicationManager, containerManager, findSourceStrategy);
```
In the constructor, we are initializing `excludeContainersDueToFailure` to empty set. That should take care of this
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java:
##########
@@ -68,6 +69,7 @@ public ContainerBalancerSelectionCriteria(
this.replicationManager = replicationManager;
this.containerManager = containerManager;
selectedContainers = new HashSet<>();
+ excludeContainersDueToFailure = new HashSet<>();
Review Comment:
In ContainerBalancerTask#initializeIteration, in the end we are creating a new object for containerBalancerSelectionCriteria
```
selectionCriteria = new ContainerBalancerSelectionCriteria(config,
nodeManager, replicationManager, containerManager, findSourceStrategy);
```
In the constructor, we are initializing `excludeContainersDueToFailure` to empty set. That should take care of this issue.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1545989946
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -856,19 +860,34 @@ private boolean moveContainer(DatanodeDetails source,
" {} failed: {}",
moveSelection.getContainerID(), source.getUuidString(),
moveSelection.getTargetNode().getUuidString(), result);
+ // add source back if move failed due to container related errors
+ if (result == MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_EXIST_IN_TARGET ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_CONTAINER_NOT_CLOSED ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_DELETION ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_REPLICATION) {
+ findSourceStrategy.addBackSourceDataNode(source);
+ }
Review Comment:
I have followed this method to handle the checks. Thank you for the suggestion. Could you please review the recent patch?
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1566906407
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -862,13 +870,24 @@ private boolean moveContainer(DatanodeDetails source,
} catch (ContainerNotFoundException e) {
LOG.warn("Could not find Container {} for container move",
containerID, e);
+ // add source back to queue as a different container can be selected in next run.
+ findSourceStrategy.addBackSourceDataNode(source);
+ // exclude the container which caused failure of move to avoid error in next run.
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
metrics.incrementNumContainerMovesFailedInLatestIteration(1);
return false;
- } catch (NodeNotFoundException | TimeoutException |
- ContainerReplicaNotFoundException e) {
+ } catch (NodeNotFoundException | TimeoutException e) {
LOG.warn("Container move failed for container {}", containerID, e);
metrics.incrementNumContainerMovesFailedInLatestIteration(1);
return false;
+ } catch (ContainerReplicaNotFoundException e) {
+ LOG.warn("Container move failed for container {}", containerID, e);
+ metrics.incrementNumContainerMovesFailedInLatestIteration(1);
+ // add source back to queue for replica not found only
+ findSourceStrategy.addBackSourceDataNode(source);
+ // exclude the container whose replica caused failure of move to avoid error in next run.
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
Review Comment:
I don't think we need to exclude this container as it's a replica related error. Balancer should get the updated list of containers in a node from node manager next time, so it's fine if we don't exclude 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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on PR #6305:
URL: https://github.com/apache/ozone/pull/6305#issuecomment-2060436042
@Tejaskriya please check the failing 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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1550057106
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java:
##########
@@ -68,6 +69,7 @@ public ContainerBalancerSelectionCriteria(
this.replicationManager = replicationManager;
this.containerManager = containerManager;
selectedContainers = new HashSet<>();
+ excludeContainersDueToFailure = new HashSet<>();
Review Comment:
excludeContainersDueToFailure holds failure in the particular iteration, and choosing the source again avoiding the container.
But in next iteration, when same source is re-considered, IMO, this list also needs cleanup. This is required to be done in,
org.apache.hadoop.hdds.scm.container.balancer.ContainerBalancerTask#initializeIteration
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1520953899
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -856,19 +860,34 @@ private boolean moveContainer(DatanodeDetails source,
" {} failed: {}",
moveSelection.getContainerID(), source.getUuidString(),
moveSelection.getTargetNode().getUuidString(), result);
+ // add source back if move failed due to container related errors
+ if (result == MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_EXIST_IN_TARGET ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_CONTAINER_NOT_CLOSED ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_DELETION ||
+ result == MoveManager.MoveResult.REPLICATION_FAIL_INFLIGHT_REPLICATION) {
+ findSourceStrategy.addBackSourceDataNode(source);
+ }
Review Comment:
@Tejaskriya After scheduling the move here:
```
if (replicationManager.getConfig().isLegacyEnabled()) {
future = replicationManager
.move(containerID, source, moveSelection.getTargetNode());
} else {
future = moveManager.move(containerID, source,
moveSelection.getTargetNode());
}
```
this method will return true here if the move hasn't failed/completed yet:
```
moveSelectionToFutureMap.put(moveSelection, future);
return true;
```
Then, `updateTargetsAndSelectionCriteria` will eventually call `findSourceStrategy.increaseSizeLeaving(source, size);`, which will add the source datanode back so it can be considered in further move selections. So, we don't need to add the source back in the asynchronous `whenComplete` handling of the `future`.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1552955192
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -563,6 +566,9 @@ private boolean processMoveSelection(DatanodeDetails source,
} catch (ContainerNotFoundException e) {
LOG.warn("Could not get container {} from Container Manager before " +
"starting a container move", containerID, e);
+ // add source back to queue as a different container can be selected in next run.
+ findSourceStrategy.addBackSourceDataNode(source);
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
Review Comment:
I missed this comment - @sumitagrawl do you want to take another look? I think @Tejaskriya has updated the PR now.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #6305:
URL: https://github.com/apache/ozone/pull/6305#discussion_r1556044087
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -563,6 +566,9 @@ private boolean processMoveSelection(DatanodeDetails source,
} catch (ContainerNotFoundException e) {
LOG.warn("Could not get container {} from Container Manager before " +
"starting a container move", containerID, e);
+ // add source back to queue as a different container can be selected in next run.
+ findSourceStrategy.addBackSourceDataNode(source);
+ selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID());
Review Comment:
If 2 different source have same container choosen to be moved to target to reduce usages at source,
s1: c5.--> t5
s2: c5 --> t6
Now s1 for c5 had some failure, so this will avoid choosing c5 container for s1 (as expected). But old logic where comment is given, it will also make c5 container not be choosed in another source s2.
--
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
Re: [PR] HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #6305:
URL: https://github.com/apache/ozone/pull/6305#issuecomment-2056218934
@siddhantsangwan @sumitagrawl following our discussion today, I have reverted the latest commit. Now the container is excluded for all DNs as ContainerNotFoundException will come up for every DN that the container is present in. For replica related intermittent failures, the container is not excluded.
I have also added some comments to mention the reason we are adding a source DN back and excluding containers. The merge conflicts are resolved.
Could you please give it a final review?
--
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