HDDS-5602. make it configurable to choose the nearest one as the target in the candidates according to networkTopology

[JacksonYao287]

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

[JacksonYao287]

@lokeshj1703 @siddhantsangwan PTAL, thanks!

[lokeshj1703]

@JacksonYao287 Thanks for working on this! The changes look good to me.
Can we add a UT if it is not a lot of effort?

[siddhantsangwan]

@JacksonYao287 It might be better to create another implementation of FindTargetStrategy that sorts potential targets by distance. FindTargetGreedy is working on a list of potential targets sorted from least utilized to most utilized.

[JacksonYao287]

@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.

[JacksonYao287]

@siddhantsangwan @lokeshj1703 i have refactored the code , please take a look!

[lokeshj1703]

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.

[JacksonYao287]

@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.

[lokeshj1703]

@JacksonYao287 Thanks for working on this! 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. With the current changes it will become part of FindSourceStrategy itself.

[lokeshj1703]

Unrelated to this PR. Since we are doing a poll here, the source datanode will not be considered again in the iteration?

[lokeshj1703]

Can you please check if this is an issue? We can create a new jira to address it if it is.

[JacksonYao287]

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

[lokeshj1703]

The issue is that we are doing a poll in potentialSources. As a result source datanode will be removed.

[siddhantsangwan]

I think this source is added back here, and that method is called here. It seems correct but the logic is a bit confusing (#2808 (comment))

[JacksonYao287]

@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?

[JacksonYao287]

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);
    })

[lokeshj1703]

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.

[JacksonYao287]

@lokeshj1703 i have updated this patch according to the comments , PTAL!

[lokeshj1703]

@JacksonYao287 Thanks for working on this! The changes look good to me. Please find my comments below.

[lokeshj1703]

Sorting would happen every time function is called. I think we can optimise for same source.

[JacksonYao287]

thanks, will do this optimization

[JacksonYao287]

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.

[lokeshj1703]

We can remove the target and re-add it I think. But we can do this in a separate PR.

[JacksonYao287]

@lokeshj1703 thanks for the review! i have update this patch according to you comments , PTAL!

[JacksonYao287]

@lokeshj1703 @siddhantsangwan i have added test case. could you help reviewing this patch?

[lokeshj1703]

@JacksonYao287 Thanks for updating the PR! The changes look good to me. +1.

[lokeshj1703]

@JacksonYao287 Thanks for the contribution! @siddhantsangwan Thanks for review! I have committed the PR to master branch.

[JacksonYao287]

thanks @lokeshj1703 @siddhantsangwan for the review!