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/10/21 08:27:10 UTC
[GitHub] [ozone] JacksonYao287 opened a new pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
JacksonYao287 opened a new pull request #2756:
URL: https://github.com/apache/ozone/pull/2756
## What changes were proposed in this pull request?
always choose the nearest one as the target in the candidates according to networkTopology
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5602
## How was this patch tested?
ut
--
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] lokeshj1703 commented on pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-999348841
@JacksonYao287 Thanks for the contribution! @siddhantsangwan Thanks for review! I have committed the PR to master branch.
--
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] lokeshj1703 commented on a change in pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r765673589
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindTargetGreedy.java
##########
@@ -109,6 +122,7 @@ private void setConfiguration(ContainerBalancerConfiguration conf) {
@Override
public ContainerMoveSelection findTargetForContainerMove(
DatanodeDetails source, Set<ContainerID> candidateContainers) {
+ sortTargetForSource(source);
Review comment:
Sorting would happen every time function is called. I think we can optimise for same source.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindTargetGreedy.java
##########
@@ -52,30 +57,19 @@
private NodeManager nodeManager;
private ContainerBalancerConfiguration config;
private Double upperLimit;
- private TreeSet<DatanodeUsageInfo> potentialTargets;
+ private Collection<DatanodeUsageInfo> potentialTargets;
+ private NetworkTopology networkTopology;
public FindTargetGreedy(
Review comment:
How about we make `FindTargetGreedy` an abstract class and have two inherited classes FindTargetByUsage and FindTargetByNetworkTopology?
--
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] JacksonYao287 commented on pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-988528503
@lokeshj1703 i have updated this patch according to the comments , PTAL!
--
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] JacksonYao287 commented on pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-948511876
@lokeshj1703 @siddhantsangwan PTAL, thanks!
--
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] JacksonYao287 commented on pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-992192051
@lokeshj1703 thanks for the review! i have update this patch according to you comments , PTAL!
--
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] lokeshj1703 commented on a change in pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r759172400
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindSourceGreedy.java
##########
@@ -100,15 +124,19 @@ public void increaseSizeLeaving(DatanodeDetails dui, long size) {
*
* @return the nex candidate source data node.
*/
- @Override
- public DatanodeDetails getNextCandidateSourceDataNode() {
+ private DatanodeDetails getNextCandidateSourceDataNode() {
if (potentialSources.isEmpty()) {
LOG.info("no more candidate source data node");
return null;
}
return potentialSources.poll().getDatanodeDetails();
Review comment:
Unrelated to this PR. Since we are doing a poll here, the source datanode will not be considered again in the iteration?
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindSourceStrategy.java
##########
@@ -30,19 +32,26 @@
public interface FindSourceStrategy {
/**
- * get the next candidate source data node according to
- * the strategy.
- *
- * @return the nex candidate source data node.
+ * set source datanode for move selection.
*/
- DatanodeDetails getNextCandidateSourceDataNode();
+ void setSourceForMoveSelection(ContainerMoveSelection moveSelection);
Review comment:
The earlier interface method seems more intuitive.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindSourceStrategy.java
##########
@@ -30,19 +32,26 @@
public interface FindSourceStrategy {
/**
- * get the next candidate source data node according to
- * the strategy.
- *
- * @return the nex candidate source data node.
+ * set source datanode for move selection.
*/
- DatanodeDetails getNextCandidateSourceDataNode();
+ void setSourceForMoveSelection(ContainerMoveSelection moveSelection);
Review comment:
Similarly for FindTargetGreedy. This seems like a setter.
--
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] JacksonYao287 commented on pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-952516983
@siddhantsangwan thanks for the review! what about adding a boolean parameter to `findTargetForContainerMove` to indicate whether to sort the potential targets from network topology, and this can be configured by user.
--
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] JacksonYao287 commented on pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-952705775
@siddhantsangwan @lokeshj1703 i have refactored the code , please take a look!
--
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] JacksonYao287 commented on a change in pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r768270855
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindTargetGreedy.java
##########
@@ -109,6 +122,7 @@ private void setConfiguration(ContainerBalancerConfiguration conf) {
@Override
public ContainerMoveSelection findTargetForContainerMove(
DatanodeDetails source, Set<ContainerID> candidateContainers) {
+ sortTargetForSource(source);
Review comment:
after a deep thought, i think we have to sort potentialTargets every time , even for same source. after a certain target is selected and a move option is scheduled to it, `sizeEntering` of it will increase, and thus the utilization will increase. so when choosing a target for even the same source, if two candidate target has the same network topology distance to the source , the priority may change according to current usageinfo.
--
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] JacksonYao287 edited a comment on pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-997546863
@lokeshj1703 @siddhantsangwan i have added test case. could you help reviewing this 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
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r737063386
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindTargetGreedy.java
##########
@@ -44,12 +46,15 @@
private ContainerManager containerManager;
private PlacementPolicy placementPolicy;
+ private StorageContainerManager scm;
public FindTargetGreedy(
Review comment:
thanks @lokeshj1703 for the review. i will update the doc and add a ut in a new commit
--
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] lokeshj1703 commented on a change in pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r736426494
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindTargetGreedy.java
##########
@@ -44,12 +46,15 @@
private ContainerManager containerManager;
private PlacementPolicy placementPolicy;
+ private StorageContainerManager scm;
public FindTargetGreedy(
Review comment:
Can we also update the javadoc for FindTargetGreedy?
--
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] JacksonYao287 commented on a change in pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r767473661
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindTargetGreedy.java
##########
@@ -109,6 +122,7 @@ private void setConfiguration(ContainerBalancerConfiguration conf) {
@Override
public ContainerMoveSelection findTargetForContainerMove(
DatanodeDetails source, Set<ContainerID> candidateContainers) {
+ sortTargetForSource(source);
Review comment:
thanks, will do this optimization
--
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] JacksonYao287 edited a comment on pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-978818822
@lokeshj1703 @siddhantsangwan , i have refactored the code again in the latest commit , please take a look!
if it looks good to you , i will then add the network topology related logic to this patch.
when selecting a target for a source, if the target meet all other conditions, i think it is better to make network topology distance as the first criteria, and then the storage usage.
--
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] JacksonYao287 commented on pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-983448371
thanks @lokeshj1703 very much for the review!
>I think it is better to have a separated ContainerBalancerSelectionCriteria as it gives flexibility later to account both source and target limitations in the criteria.
for now , the only role of ContainerBalancerSelectionCriteria is to get Candidate Containers. but i agree with you. may be later in the future, we need to do some flexible work in ContainerBalancerSelectionCriteria. so let us keep it for now and i will cancel this refactor.
so in this patch , i will only do the network topology related work. so i want to hear your opinion, should we sorting candidate targets by network first, or make it configurable?
if we sort candidate targets by network first, we will then use a common list instead of current Treeset to hold all the candidate targets, and we should sort all the candidate targets for each source datanode with a function , for example:
```
List<DatanodeUsageInfo> potentialTargets;
DatanodeUsageInfo source;
NetworkTopology nt = scm.getClusterMap();
//for each source, we should sort
potentialTargets.sort((a, b) -> {
// sort by network topology first
int ret = 0;
int distancetoA = nt.getDistanceCost(source, a);
int distancetoB = nt.getDistanceCost(source, b);
ret = distancetoA - distancetoB;
if (ret != 0) {
return ret;
}
// if network distance is equal, sort by usage
double currentUsageOfA = a.calculateUtilization(
sizeEnteringNode.get(a.getDatanodeDetails()));
double currentUsageOfB = b.calculateUtilization(
sizeEnteringNode.get(b.getDatanodeDetails()));
ret = Double.compare(currentUsageOfA, currentUsageOfB);
if (ret != 0) {
return ret;
}
UUID uuidA = a.getDatanodeDetails().getUuid();
UUID uuidB = b.getDatanodeDetails().getUuid();
return uuidA.compareTo(uuidB);
})
```
--
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] JacksonYao287 edited a comment on pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-983448371
thanks @lokeshj1703 very much for the review!
>I think it is better to have a separated ContainerBalancerSelectionCriteria as it gives flexibility later to account both source and target limitations in the criteria.
for now , the only role of ContainerBalancerSelectionCriteria is to get Candidate Containers. but i agree with you. may be later in the future, we need to do some flexible work in ContainerBalancerSelectionCriteria. so let us keep it for now and i will cancel this refactor.
so in this patch , i will only do the network topology related work. so i want to hear your opinion, should we sorting candidate targets by network first, or make it configurable?
if we sort candidate targets by network first, we will then use a common list instead of current Treeset to hold all the candidate targets, and we should sort all the candidate targets for each source datanode with a function , for example:
```
List<DatanodeUsageInfo> potentialTargets;
DatanodeUsageInfo source;
NetworkTopology nt = scm.getClusterMap();
//for each source, we should sort
potentialTargets.sort((a, b) -> {
// sort by network topology first
int ret = 0;
int distancetoA = nt.getDistanceCost(source, a);
int distancetoB = nt.getDistanceCost(source, b);
ret = distancetoA - distancetoB;
if (ret != 0) {
return ret;
}
// if network distance is equal, sort by usage
double currentUsageOfA = a.calculateUtilization(
sizeEnteringNode.get(a.getDatanodeDetails()));
double currentUsageOfB = b.calculateUtilization(
sizeEnteringNode.get(b.getDatanodeDetails()));
ret = Double.compare(currentUsageOfA, currentUsageOfB);
if (ret != 0) {
return ret;
}
UUID uuidA = a.getDatanodeDetails().getUuid();
UUID uuidB = b.getDatanodeDetails().getUuid();
return uuidA.compareTo(uuidB);
})
```
--
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] JacksonYao287 commented on a change in pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r773674320
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindSourceGreedy.java
##########
@@ -100,15 +124,19 @@ public void increaseSizeLeaving(DatanodeDetails dui, long size) {
*
* @return the nex candidate source data node.
*/
- @Override
- public DatanodeDetails getNextCandidateSourceDataNode() {
+ private DatanodeDetails getNextCandidateSourceDataNode() {
if (potentialSources.isEmpty()) {
LOG.info("no more candidate source data node");
return null;
}
return potentialSources.poll().getDatanodeDetails();
Review comment:
this is not an issue. this happens when i tried to refactor the findTargetStrategy and remove ContainerBalancerSelectionCriteria. i have canceled this refactor, so `getNextCandidateSourceDataNode ` is public 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
[GitHub] [ozone] siddhantsangwan commented on a change in pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r773704328
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindSourceGreedy.java
##########
@@ -100,15 +124,19 @@ public void increaseSizeLeaving(DatanodeDetails dui, long size) {
*
* @return the nex candidate source data node.
*/
- @Override
- public DatanodeDetails getNextCandidateSourceDataNode() {
+ private DatanodeDetails getNextCandidateSourceDataNode() {
if (potentialSources.isEmpty()) {
LOG.info("no more candidate source data node");
return null;
}
return potentialSources.poll().getDatanodeDetails();
Review comment:
I think this source is added back [here](https://github.com/apache/ozone/blob/e01b47184818365dcb7baf528fb2d874209b6b47/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindSourceGreedy.java#L90), and that method is called [here](https://github.com/apache/ozone/blob/e01b47184818365dcb7baf528fb2d874209b6b47/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java#L619). It seems correct but the logic is a bit confusing (https://github.com/apache/ozone/pull/2808#discussion_r773675544)
--
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] lokeshj1703 commented on a change in pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r773657242
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindTargetGreedy.java
##########
@@ -109,6 +122,7 @@ private void setConfiguration(ContainerBalancerConfiguration conf) {
@Override
public ContainerMoveSelection findTargetForContainerMove(
DatanodeDetails source, Set<ContainerID> candidateContainers) {
+ sortTargetForSource(source);
Review comment:
We can remove the target and re-add it I think. But we can do this in a separate PR.
--
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] lokeshj1703 merged pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
lokeshj1703 merged pull request #2756:
URL: https://github.com/apache/ozone/pull/2756
--
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] lokeshj1703 commented on a change in pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r773692241
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindSourceGreedy.java
##########
@@ -100,15 +124,19 @@ public void increaseSizeLeaving(DatanodeDetails dui, long size) {
*
* @return the nex candidate source data node.
*/
- @Override
- public DatanodeDetails getNextCandidateSourceDataNode() {
+ private DatanodeDetails getNextCandidateSourceDataNode() {
if (potentialSources.isEmpty()) {
LOG.info("no more candidate source data node");
return null;
}
return potentialSources.poll().getDatanodeDetails();
Review comment:
The issue is that we are doing a poll in potentialSources. As a result source datanode will be removed.
--
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] JacksonYao287 commented on pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-978818822
@lokeshj1703 @siddhantsangwan , i have refactored the code again in the latest commit , please take a look!
if it looks good to you , i will then add the network topology related logic to this patch.
when selecting a target for a source, i think it is better to make network topology distance as the first criteria, and then the storage usage.
--
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] lokeshj1703 commented on pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-983466845
I think configurable is better. It would be preferable to choose most under utilised nodes first in cases like addition of new nodes or highly unbalanced cluster.
--
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] JacksonYao287 commented on a change in pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r767473912
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindTargetGreedy.java
##########
@@ -52,30 +57,19 @@
private NodeManager nodeManager;
private ContainerBalancerConfiguration config;
private Double upperLimit;
- private TreeSet<DatanodeUsageInfo> potentialTargets;
+ private Collection<DatanodeUsageInfo> potentialTargets;
+ private NetworkTopology networkTopology;
public FindTargetGreedy(
Review comment:
sure , will do this
--
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] JacksonYao287 commented on pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-997546863
@lokeshj1703 i have added test case. could you help reviewing this 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
[GitHub] [ozone] lokeshj1703 commented on pull request #2756: HDDS-5602. always choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-952797052
I would recommend using an Interface. @JacksonYao287 @siddhantsangwan Let's first finalise how the interface should look like and how to integrate it with balancer. Please see if we would like to use existing selection criteria/strategy.
--
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] lokeshj1703 commented on a change in pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r773657587
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindSourceGreedy.java
##########
@@ -100,15 +124,19 @@ public void increaseSizeLeaving(DatanodeDetails dui, long size) {
*
* @return the nex candidate source data node.
*/
- @Override
- public DatanodeDetails getNextCandidateSourceDataNode() {
+ private DatanodeDetails getNextCandidateSourceDataNode() {
if (potentialSources.isEmpty()) {
LOG.info("no more candidate source data node");
return null;
}
return potentialSources.poll().getDatanodeDetails();
Review comment:
Can you please check if this is an issue? We can create a new jira to address it if it is.
--
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] JacksonYao287 commented on pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#issuecomment-999361241
thanks @lokeshj1703 @siddhantsangwan for the 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
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2756: HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2756:
URL: https://github.com/apache/ozone/pull/2756#discussion_r773837645
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/FindSourceGreedy.java
##########
@@ -100,15 +124,19 @@ public void increaseSizeLeaving(DatanodeDetails dui, long size) {
*
* @return the nex candidate source data node.
*/
- @Override
- public DatanodeDetails getNextCandidateSourceDataNode() {
+ private DatanodeDetails getNextCandidateSourceDataNode() {
if (potentialSources.isEmpty()) {
LOG.info("no more candidate source data node");
return null;
}
return potentialSources.poll().getDatanodeDetails();
Review comment:
@lokeshj1703 @siddhantsangwan
```
private void incSizeSelectedForMoving(DatanodeDetails source,
ContainerMoveSelection moveSelection) {
````````
// update sizeLeavingNode map with the recent moveSelection
findSourceStrategy.increaseSizeLeaving(source, size);
// update sizeEnteringNode map with the recent moveSelection
findTargetStrategy.increaseSizeEntering(target, size);
}
```
```
FindSourceGreedy#increaseSizeLeaving
public void increaseSizeLeaving(DatanodeDetails dui, long size) {
Long currentSize = sizeLeavingNode.get(dui);
if(currentSize != null) {
sizeLeavingNode.put(dui, currentSize + size);
//reorder according to the latest sizeLeavingNode
potentialSources.add(nodeManager.getUsageInfo(dui));
return;
}
LOG.warn("Cannot find datanode {} in candidate source datanodes",
dui.getUuid());
}
```
if we can find a target and a container for this source datanode, then `incSizeSelectedForMoving` will be definitely called , so this source data node will be added back to that priority queue and re-sort again.
if we can not find any target and container for this source datanode , what we should do is just removing it.
so i don`t think this is an issue. does this explanation make sense to you?
--
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