[GitHub] [helix] xyuanlu edited a comment on pull request #1307: Add TrieClusterTopology for retrieving hierarchical topology

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

xyuanlu edited a comment on pull request #1307:
URL: https://github.com/apache/helix/pull/1307#issuecomment-679405672


   Minor correction here. 
   I think the enum Type mentioned above is used only when cluster level clusterConfig.Topology is not defined. Otherwise, we still parse the topology definition in cluster config.
   
   In function getClusterTopologySetting:
   ```
   private static ClusterTopologyConfig getClusterTopologySetting(ClusterConfig clusterConfig) {
       ClusterTopologyConfig clusterTopologyConfig = new ClusterTopologyConfig();
          ...
         if (topologyDef != null) {
           // We parse "/zone/rack/sub-rack/instance" into  ["zone", "rack", "sub-rack", "instance"].
         } else {
           // Use default cluster topology definition, i,e. /root/zone/instance
           clusterTopologyConfig.endNodeType = Types.INSTANCE.name();
           clusterTopologyConfig.faultZoneType = Types.ZONE.name();
         }
         ...
     }
   ```
   
   
   In my understanding, current Topology.java has similar functions like validateAndOrderDomain and the constructor  TrieClusterTopology. 
   Topology::getClusterTopologySetting returns a parsed list of cluster topology as the constructor here. 
   There are similar logic of validating the instance topology config aligns with cluster's config in computeInstanceTopologyMapHelper. 
   
   IMHO, I personally prefer reusing current code. Also I think its better to read from the tree since we already have the tree created for LB.
   
   Thanks,
   Xiaoyuan 


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org