[GitHub] [ozone] sodonnel commented on pull request #3606: HDDS-7015. Failures while choosing nodes for creating container

[GitBox <gi...@apache.org>]

sodonnel commented on PR #3606:
URL: https://github.com/apache/ozone/pull/3606#issuecomment-1188976178

   While the change here will work, I do wonder if the same problem exists in other placement policies. I also wonder if a similar problem could exist with favoredNodes, although we don't seem to use it right now, at least in the EC flow.
   
   Would it be better to provide a new method on SCMNodeManager (and its interface) to lookup the network location for a node that can be used within the Common Placement Policy, as it already has a reference to NodeManager. NodeManager already has the logic to resolve a host when it registers (see the register method).
   
   Then in SCMCommonPlacementPolicy, change the method:
   
   ```
   @Override
     public List<DatanodeDetails> chooseDatanodes(
         List<DatanodeDetails> excludedNodes, List<DatanodeDetails> favoredNodes,
         int nodesRequired, long metadataSizeRequired, long dataSizeRequired)
         throws SCMException {
   ```
   
   To look like:
   
   ```
   
     public List<DatanodeDetails> final chooseDatanodes(
         List<DatanodeDetails> excludedNodes, List<DatanodeDetails> favoredNodes,
         int nodesRequired, long metadataSizeRequired, long dataSizeRequired)
         throws SCMException {
         fixedFavoredNodes = setNetworkLocation(favoredNodes);
         fixedExcludedNodes = setNetworkLocation(excludedNodes);
         chooseDatanodesInternal(...);
     }
     
     protected List<DatanodeDetails> chooseDatanodesInternal(....) {
        // existing logic the original chooseDatanode
        // Sub-classes must override here to provide their own implementation of 
        // chooseDatanodes, ensuring the favored / excluded node lists always have their
        // network location resolved.
     }
         
   ```
   
   This way, each implementation must override the protected method and the common code will always get executed. It would be even better if we could make chooseDatanodesInternal abstract so they must provide an implementation, but I am not sure we can do that with the way things are structured, as we have Interface -> CommonImpl -> Other impls.


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