xyuanlu commented 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. 
   
   IMHO, I personally prefer reusing current code. 


----------------------------------------------------------------
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:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to