jiajunwang commented on a change in pull request #1043:
URL: https://github.com/apache/helix/pull/1043#discussion_r438481995
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -81,62 +75,43 @@ public Topology(final List<String> allNodes, final
List<String> liveNodes,
throw new HelixException(String.format("Config for instances %s is not
found!",
_allInstances.removeAll(_instanceConfigMap.keySet())));
}
-
_clusterConfig = clusterConfig;
- _types = new LinkedHashSet<>();
- _topologyAwareEnabled = clusterConfig.isTopologyAwareEnabled();
- if (_topologyAwareEnabled) {
+ if (clusterConfig.isTopologyAwareEnabled()) {
String topologyDef = _clusterConfig.getTopology();
if (topologyDef != null) {
// Customized cluster topology definition is configured.
- String[] types = topologyDef.trim().split("/");
- for (int i = 0; i < types.length; i++) {
- if (types[i].length() != 0) {
- _types.add(types[i]);
+ String[] topologyStr = topologyDef.trim().split("/");
+ int lastValidTypeIdx = topologyStr.length - 1;
+ while(lastValidTypeIdx >= 0) {
+ if (topologyStr[lastValidTypeIdx].length() != 0) {
+ break;
}
+ --lastValidTypeIdx;
}
- if (_types.size() == 0) {
- logger.error("Invalid cluster topology definition " + topologyDef);
+ if (lastValidTypeIdx < 0) {
throw new HelixException("Invalid cluster topology definition " +
topologyDef);
- } else {
- String lastType = null;
- for (String type : _types) {
- _defaultDomainPathValues.put(type, "Helix_default_" + type);
- lastType = type;
- }
- _endNodeType = lastType;
- _faultZoneType = clusterConfig.getFaultZoneType();
- if (_faultZoneType == null) {
- _faultZoneType = _endNodeType;
- }
- if (!_types.contains(_faultZoneType)) {
- throw new HelixException(String
- .format("Invalid fault zone type %s, not present in topology
definition %s.",
- _faultZoneType, topologyDef));
- }
- _useDefaultTopologyDef = false;
+ }
+ _endNodeType = topologyStr[lastValidTypeIdx];
+ _faultZoneType = clusterConfig.getFaultZoneType();
+ if (_faultZoneType == null) {
+ _faultZoneType = _endNodeType;
+ }
+ if (Arrays.stream(topologyStr).noneMatch(type ->
type.equals(_faultZoneType))) {
Review comment:
This operation is more expansive than the original code. What's the
purpose of modifying this part? It does not seem to be simpler.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -55,16 +55,10 @@
private final List<String> _liveInstances;
private final Map<String, InstanceConfig> _instanceConfigMap;
private final ClusterConfig _clusterConfig;
- private final boolean _topologyAwareEnabled;
+ private static final String DEFAULT_PATH_PREFIX = "Helix_default_";
Review comment:
Why it is PATH_PREFIX, I think it should be the VALUE_PREFIX. Of course,
the value prefix is not a good name either. Let's call it
DEFAULT_DOMAIN_PREFIX. And add some comments to indicate what's the usage.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -81,62 +75,43 @@ public Topology(final List<String> allNodes, final
List<String> liveNodes,
throw new HelixException(String.format("Config for instances %s is not
found!",
_allInstances.removeAll(_instanceConfigMap.keySet())));
}
-
_clusterConfig = clusterConfig;
- _types = new LinkedHashSet<>();
- _topologyAwareEnabled = clusterConfig.isTopologyAwareEnabled();
- if (_topologyAwareEnabled) {
+ if (clusterConfig.isTopologyAwareEnabled()) {
String topologyDef = _clusterConfig.getTopology();
if (topologyDef != null) {
// Customized cluster topology definition is configured.
- String[] types = topologyDef.trim().split("/");
- for (int i = 0; i < types.length; i++) {
- if (types[i].length() != 0) {
- _types.add(types[i]);
+ String[] topologyStr = topologyDef.trim().split("/");
+ int lastValidTypeIdx = topologyStr.length - 1;
+ while(lastValidTypeIdx >= 0) {
+ if (topologyStr[lastValidTypeIdx].length() != 0) {
+ break;
}
+ --lastValidTypeIdx;
}
- if (_types.size() == 0) {
- logger.error("Invalid cluster topology definition " + topologyDef);
+ if (lastValidTypeIdx < 0) {
throw new HelixException("Invalid cluster topology definition " +
topologyDef);
- } else {
- String lastType = null;
- for (String type : _types) {
- _defaultDomainPathValues.put(type, "Helix_default_" + type);
- lastType = type;
- }
- _endNodeType = lastType;
- _faultZoneType = clusterConfig.getFaultZoneType();
- if (_faultZoneType == null) {
- _faultZoneType = _endNodeType;
- }
- if (!_types.contains(_faultZoneType)) {
- throw new HelixException(String
- .format("Invalid fault zone type %s, not present in topology
definition %s.",
- _faultZoneType, topologyDef));
- }
- _useDefaultTopologyDef = false;
+ }
+ _endNodeType = topologyStr[lastValidTypeIdx];
+ _faultZoneType = clusterConfig.getFaultZoneType();
+ if (_faultZoneType == null) {
+ _faultZoneType = _endNodeType;
+ }
+ if (Arrays.stream(topologyStr).noneMatch(type ->
type.equals(_faultZoneType))) {
Review comment:
Note this operation is called many times during the rebalance, so any
incremental delay is impactful. Must be very careful, or the rebalance latency
will be increased.
##########
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 {}",
+ domain, topologyKeys.toString());
}
- pathValueMap.put(type, value);
- }
-
- int weight = insConfig.getWeight();
- if (weight < 0 || weight == InstanceConfig.WEIGHT_NOT_SET) {
- weight = DEFAULT_NODE_WEIGHT;
}
- root = addEndNode(root, ins, pathValueMap, weight, _liveInstances);
+ } else {
+ instanceTopologyMap.put(Types.INSTANCE.name(), instance);
}
- return root;
+ return instanceTopologyMap;
}
-
/**
* Add an end node to the tree, create all the paths to the leaf node if not
present.
+ * Input param 'pathNameMap' must have a certain order where the order of
the keys should be
Review comment:
In this case, please convert the pathNameMap to be a LinkedHashMap input.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -81,62 +75,43 @@ public Topology(final List<String> allNodes, final
List<String> liveNodes,
throw new HelixException(String.format("Config for instances %s is not
found!",
_allInstances.removeAll(_instanceConfigMap.keySet())));
}
-
_clusterConfig = clusterConfig;
- _types = new LinkedHashSet<>();
- _topologyAwareEnabled = clusterConfig.isTopologyAwareEnabled();
- if (_topologyAwareEnabled) {
+ if (clusterConfig.isTopologyAwareEnabled()) {
String topologyDef = _clusterConfig.getTopology();
if (topologyDef != null) {
// Customized cluster topology definition is configured.
- String[] types = topologyDef.trim().split("/");
- for (int i = 0; i < types.length; i++) {
- if (types[i].length() != 0) {
- _types.add(types[i]);
+ String[] topologyStr = topologyDef.trim().split("/");
+ int lastValidTypeIdx = topologyStr.length - 1;
+ while(lastValidTypeIdx >= 0) {
Review comment:
This logic cannot handle the following case correctly,
"/domain/rack///node/"
##########
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();
Review comment:
Why not directly call domainAsMap = instanceConfig.getDomainAsMap();?
##########
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.
Review comment:
This comment is not right.
##########
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 =
Review comment:
This should be LinkedHashMap
##########
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:
instance -> instanceName
##########
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:
Could you just input the String[] topologyStr (or a LinkedHashSet which
I would recommend) to this method so you don't do the calculation again?
##########
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:
Mention here that the default domain value has been used. Otherwise,
this warning looks like a major problem.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -81,62 +75,43 @@ public Topology(final List<String> allNodes, final
List<String> liveNodes,
throw new HelixException(String.format("Config for instances %s is not
found!",
_allInstances.removeAll(_instanceConfigMap.keySet())));
}
-
_clusterConfig = clusterConfig;
- _types = new LinkedHashSet<>();
Review comment:
I would prefer to keep it. A LinkedHashSet or ArrayList would be much
easier to handle compared with array.
##########
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
Review comment:
This is duplicate code, let's have a private method to check if the node
is enabled or not.
----------------------------------------------------------------
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]
