xyuanlu commented on a change in pull request #1043:
URL: https://github.com/apache/helix/pull/1043#discussion_r439650362
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -212,123 +187,129 @@ 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();
- if (zone == null) {
- // we have the hierarchy style of domain id for instance.
- if (config.getInstanceEnabled() &&
(_clusterConfig.getDisabledInstances() == null
- || !_clusterConfig.getDisabledInstances().containsKey(ins))) {
- // 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));
- } 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;
- }
-
+ for (String instance : _allInstances) {
+ InstanceConfig insConfig = _instanceConfigMap.get(instance);
+ Map<String, String> instanceTopologyMap =
+ computeInstanceTopologyMap(_clusterConfig, instance, insConfig);
+ if (instanceTopologyMap != null) {
+ int weight = insConfig.getWeight();
+ if (weight < 0 || weight == InstanceConfig.WEIGHT_NOT_SET) {
+ weight = DEFAULT_NODE_WEIGHT;
}
- pathValueMap.put(Types.ZONE.name(), zone);
+ addEndNode(root, instance, instanceTopologyMap, weight,
_liveInstances);
}
- 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).
+ * This function returns a LinkedHashMap<String, String> object representing
+ * the topology path for an instance.
+ * LinkedHashMap is used here since the order of the path needs to be
preserved
+ * when creating the topology tree.
+ *
+ * @param clusterConfig clusterConfig of the cluster.
+ * @param instance Name of the given instance.
+ * @param instanceConfig instanceConfig of the instance.
+ * @return an LinkedHashMap object representing the topology path for the
input instance.
*/
- 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;
+ private LinkedHashMap<String, String>
computeInstanceTopologyMap(ClusterConfig clusterConfig,
+ String instance, InstanceConfig instanceConfig) {
+ LinkedHashMap<String, String> instanceTopologyMap = new LinkedHashMap<>();
+ if (clusterConfig.isTopologyAwareEnabled()) {
+ String topologyDef = clusterConfig.getTopology();
+ if (topologyDef == null) {
+ // Return a ordered 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 (instanceConfig.getInstanceEnabled() &&
(clusterConfig.getDisabledInstances() == null
+ || !clusterConfig.getDisabledInstances().containsKey(instance)))
{
+ // if enabled instance missing ZONE_ID information, fail the
rebalance.
+ throw new HelixException(String
+ .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("ZONE_ID for instance {} is not set, ignore the
instance!", instance);
+ return null;
+ }
}
- }
-
- 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()) {
+ instanceTopologyMap.put(Types.ZONE.name(), zone);
+ instanceTopologyMap.put(Types.INSTANCE.name(), instance);
+ } else {
+ /*
+ * Return a ordered map representing the instance path. The topology
order is defined in
+ * ClusterConfig.topology.
+ */
+ 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, fail 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 null;
+ }
+ }
+ 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("/");
Review comment:
Updated.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -212,123 +187,129 @@ 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();
- if (zone == null) {
- // we have the hierarchy style of domain id for instance.
- if (config.getInstanceEnabled() &&
(_clusterConfig.getDisabledInstances() == null
- || !_clusterConfig.getDisabledInstances().containsKey(ins))) {
- // 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));
- } 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;
- }
-
+ for (String instance : _allInstances) {
+ InstanceConfig insConfig = _instanceConfigMap.get(instance);
+ Map<String, String> instanceTopologyMap =
+ computeInstanceTopologyMap(_clusterConfig, instance, insConfig);
+ if (instanceTopologyMap != null) {
+ int weight = insConfig.getWeight();
+ if (weight < 0 || weight == InstanceConfig.WEIGHT_NOT_SET) {
+ weight = DEFAULT_NODE_WEIGHT;
}
- pathValueMap.put(Types.ZONE.name(), zone);
+ addEndNode(root, instance, instanceTopologyMap, weight,
_liveInstances);
}
- 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).
+ * This function returns a LinkedHashMap<String, String> object representing
+ * the topology path for an instance.
+ * LinkedHashMap is used here since the order of the path needs to be
preserved
+ * when creating the topology tree.
+ *
+ * @param clusterConfig clusterConfig of the cluster.
+ * @param instance Name of the given instance.
+ * @param instanceConfig instanceConfig of the instance.
+ * @return an LinkedHashMap object representing the topology path for the
input instance.
*/
- 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;
+ private LinkedHashMap<String, String>
computeInstanceTopologyMap(ClusterConfig clusterConfig,
+ String instance, InstanceConfig instanceConfig) {
+ LinkedHashMap<String, String> instanceTopologyMap = new LinkedHashMap<>();
+ if (clusterConfig.isTopologyAwareEnabled()) {
+ String topologyDef = clusterConfig.getTopology();
+ if (topologyDef == null) {
+ // Return a ordered 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 (instanceConfig.getInstanceEnabled() &&
(clusterConfig.getDisabledInstances() == null
+ || !clusterConfig.getDisabledInstances().containsKey(instance)))
{
+ // if enabled instance missing ZONE_ID information, fail the
rebalance.
+ throw new HelixException(String
+ .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("ZONE_ID for instance {} is not set, ignore the
instance!", instance);
+ return null;
+ }
}
- }
-
- 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()) {
+ instanceTopologyMap.put(Types.ZONE.name(), zone);
+ instanceTopologyMap.put(Types.INSTANCE.name(), instance);
+ } else {
+ /*
+ * Return a ordered map representing the instance path. The topology
order is defined in
+ * ClusterConfig.topology.
+ */
+ 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, fail 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 null;
+ }
+ }
+ 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("/");
+ int numOfmatchedKeys = 0;
+ 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 = DEFAULT_PATH_PREFIX + key;
+ } else {
+ numOfmatchedKeys++;
+ }
+ instanceTopologyMap.put(key, value);
+ }
+ }
+ if (numOfmatchedKeys != domainAsMap.size()) {
+ logger.warn(
+ "Key-value pairs in InstanceConfig.Domain {} do not align with
keys in ClusterConfig.Topology {}",
Review comment:
Updated.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -212,123 +187,129 @@ 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();
- if (zone == null) {
- // we have the hierarchy style of domain id for instance.
- if (config.getInstanceEnabled() &&
(_clusterConfig.getDisabledInstances() == null
- || !_clusterConfig.getDisabledInstances().containsKey(ins))) {
- // 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));
- } 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;
- }
-
+ for (String instance : _allInstances) {
+ InstanceConfig insConfig = _instanceConfigMap.get(instance);
+ Map<String, String> instanceTopologyMap =
+ computeInstanceTopologyMap(_clusterConfig, instance, insConfig);
+ if (instanceTopologyMap != null) {
+ int weight = insConfig.getWeight();
+ if (weight < 0 || weight == InstanceConfig.WEIGHT_NOT_SET) {
+ weight = DEFAULT_NODE_WEIGHT;
}
- pathValueMap.put(Types.ZONE.name(), zone);
+ addEndNode(root, instance, instanceTopologyMap, weight,
_liveInstances);
}
- 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).
+ * This function returns a LinkedHashMap<String, String> object representing
+ * the topology path for an instance.
+ * LinkedHashMap is used here since the order of the path needs to be
preserved
+ * when creating the topology tree.
+ *
+ * @param clusterConfig clusterConfig of the cluster.
+ * @param instance Name of the given instance.
+ * @param instanceConfig instanceConfig of the instance.
+ * @return an LinkedHashMap object representing the topology path for the
input instance.
*/
- 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;
+ private LinkedHashMap<String, String>
computeInstanceTopologyMap(ClusterConfig clusterConfig,
+ String instance, InstanceConfig instanceConfig) {
Review comment:
Updated.
----------------------------------------------------------------
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]
