You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/06/01 00:12:38 UTC
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1043: Code clean up Topology.java
zhangmeng916 commented on a change in pull request #1043:
URL: https://github.com/apache/helix/pull/1043#discussion_r432998959
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -212,123 +186,106 @@ private static Node cloneTree(Node root, Map<Node, Integer> newNodeWeight, Set<N
return newRoot;
}
- /**
- * Creates a tree representing the cluster structure using default cluster topology definition
- * (i,e no topology definition given and no domain id set).
- */
- private Node createClusterTreeWithDefaultTopologyDef() {
+ private Node createClusterTree() {
// root
Node root = new Node();
root.setName("root");
root.setId(computeId("root"));
root.setType(Types.ROOT.name());
- for (String ins : _allInstances) {
- InstanceConfig config = _instanceConfigMap.get(ins);
- Map<String, String> pathValueMap = new HashMap<>();
- if (_topologyAwareEnabled) {
- String zone = config.getZoneId();
+ for (String instance : _allInstances) {
+ InstanceConfig insConfig = _instanceConfigMap.get(instance);
+ LinkedHashMap<String, String> instanceTopologyMap = new LinkedHashMap<>();
+ if (computeInstanceTopologyMap(_clusterConfig, instance, insConfig, instanceTopologyMap)) {
+ int weight = insConfig.getWeight();
+ if (weight < 0 || weight == InstanceConfig.WEIGHT_NOT_SET) {
+ weight = DEFAULT_NODE_WEIGHT;
+ }
+ addEndNode(root, instance, instanceTopologyMap, weight, _liveInstances);
+ }
+ }
+ return root;
+ }
+
+ private boolean computeInstanceTopologyMap(ClusterConfig clusterConfig, String instance,
+ InstanceConfig instanceConfig, LinkedHashMap<String, String> instanceTopologyMap) {
+ if (clusterConfig.isTopologyAwareEnabled()) {
+ String topologyDef = clusterConfig.getTopology();
+ if (topologyDef == null) {
+ // Return a map using default cluster topology definition, i,e. /root/zone/instance
+ String zone = instanceConfig.getZoneId();
if (zone == null) {
// we have the hierarchy style of domain id for instance.
- if (config.getInstanceEnabled() && (_clusterConfig.getDisabledInstances() == null
- || !_clusterConfig.getDisabledInstances().containsKey(ins))) {
+ if (instanceConfig.getInstanceEnabled() && (clusterConfig.getDisabledInstances() == null
+ || !clusterConfig.getDisabledInstances().containsKey(instance))) {
// if enabled instance missing ZONE_ID information, fails the rebalance.
throw new HelixException(String
- .format("ZONE_ID for instance %s is not set, failed the topology-aware placement!",
- ins));
+ .format("ZONE_ID for instance %s is not set, fail the topology-aware placement!",
+ instance));
} else {
// if the disabled instance missing ZONE setting, ignore it should be fine.
- logger.warn(String
- .format("ZONE_ID for instance %s is not set, failed the topology-aware placement!",
- ins));
- continue;
+ logger.warn("ZONE_ID for instance {} is not set, ignore the instance!", instance);
+ return false;
}
-
}
- pathValueMap.put(Types.ZONE.name(), zone);
- }
- pathValueMap.put(Types.INSTANCE.name(), ins);
- int weight = config.getWeight();
- if (weight < 0 || weight == InstanceConfig.WEIGHT_NOT_SET) {
- weight = DEFAULT_NODE_WEIGHT;
- }
- root = addEndNode(root, ins, pathValueMap, weight, _liveInstances);
- }
- return root;
- }
-
- /**
- * Creates a tree representing the cluster structure using default cluster topology definition
- * (i,e no topology definition given and no domain id set).
- */
- private Node createClusterTreeWithCustomizedTopology() {
- // root
- Node root = new Node();
- root.setName("root");
- root.setId(computeId("root"));
- root.setType(Types.ROOT.name());
-
- for (String ins : _allInstances) {
- InstanceConfig insConfig = _instanceConfigMap.get(ins);
- String domain = insConfig.getDomainAsString();
- if (domain == null) {
- if (insConfig.getInstanceEnabled() && (_clusterConfig.getDisabledInstances() == null
- || !_clusterConfig.getDisabledInstances().containsKey(ins))) {
- // if enabled instance missing domain information, fails the rebalance.
- throw new HelixException(String
- .format("Domain for instance %s is not set, failed the topology-aware placement!",
- ins));
- } else {
- // if the disabled instance missing domain setting, ignore it should be fine.
- logger
- .warn(String.format("Domain for instance %s is not set, ignore the instance!", ins));
- continue;
+ instanceTopologyMap.put(Types.ZONE.name(), zone);
+ instanceTopologyMap.put(Types.INSTANCE.name(), instance);
+ } else {
+ /*
+ * Return a map representing the cluster structure using cluster topology defined in
+ * TOPOLOGY in ClusterConfig.
+ */
+ String domain = instanceConfig.getDomainAsString();
+ if (domain == null || domain.isEmpty()) {
+ if (instanceConfig.getInstanceEnabled() && (clusterConfig.getDisabledInstances() == null
+ || !clusterConfig.getDisabledInstances().containsKey(instance))) {
+ // if enabled instance missing domain information, fails the rebalance.
+ throw new HelixException(String
+ .format("Domain for instance %s is not set, fail the topology-aware placement!",
+ instance));
+ } else {
+ // if the disabled instance missing domain setting, ignore it should be fine.
+ logger.warn("Domain for instance {} is not set, ignore the instance!", instance);
+ return false;
+ }
}
- }
-
- String[] pathPairs = domain.trim().split(",");
- Map<String, String> pathValueMap = new HashMap<>();
- for (String pair : pathPairs) {
- String[] values = pair.trim().split("=");
- if (values.length != 2 || values[0].isEmpty() || values[1].isEmpty()) {
+ Map<String, String> domainAsMap;
+ try {
+ domainAsMap = instanceConfig.getDomainAsMap();
+ } catch (IllegalArgumentException e) {
throw new HelixException(String.format(
- "Domain-Value pair %s for instance %s is not valid, failed the topology-aware placement!",
- pair, ins));
+ "Domain %s for instance %s is not valid, fail the topology-aware placement!",
+ domain, instance));
}
- String type = values[0];
- String value = values[1];
-
- if (!_types.contains(type)) {
- logger.warn(String
- .format("Path %s defined in domain of instance %s not recognized, ignored!", pair,
- ins));
- continue;
+ String[] topologyKeys = topologyDef.trim().split("/");
+ for (String key : topologyKeys) {
+ if (!key.isEmpty()) {
+ // if a key does not exist in the instance domain config, apply the default domain value.
+ String value = domainAsMap.get(key);
+ if (value == null || value.length()==0) {
+ value = _defaultPathPrefix + key;
+ }
+ instanceTopologyMap.put(key, value);
Review comment:
I'd like to know the rationale of setting default value. Basically if the "domain" field is not set, we throw an exception, but if the "domain" is set with a random value, we think it is fine and give it a default zone value. How do we know it doesn't cause problem for rebalance.
----------------------------------------------------------------
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