jiajunwang commented on a change in pull request #1043:
URL: https://github.com/apache/helix/pull/1043#discussion_r439685972
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -212,123 +190,124 @@ 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;
- }
-
+ // TODO: Currently we add disabled instance to the topology tree. Since
they are not considered
+ // TODO: in relabalnce, maybe we should skip adding them to the tree for
consistence.
+ for (String instanceName : _allInstances) {
+ InstanceConfig insConfig = _instanceConfigMap.get(instanceName);
+ LinkedHashMap<String, String> instanceTopologyMap;
Review comment:
nit, just put in the try block. It is not used outside.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -81,62 +78,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();
+ _clusterTopologyKeys = new LinkedHashSet<>();
- 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[] topologyKeys = topologyDef.trim().split("/");
+ int lastValidTypeIdx = 0;
+ for (int i = 0; i < topologyKeys.length; i++) {
+ if (topologyKeys[i].length() != 0) {
+ _clusterTopologyKeys.add(topologyKeys[i]);
+ lastValidTypeIdx = i;
}
}
- if (_types.size() == 0) {
- logger.error("Invalid cluster topology definition " + topologyDef);
+ if (_clusterTopologyKeys.size() == 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 = topologyKeys[lastValidTypeIdx];
Review comment:
Does "_clusterTopologyKeys.get(_clusterTopologyKeys.size() - 1)" works
the same? But you won't need lastValidTypeIdx in that way.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -81,62 +78,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();
+ _clusterTopologyKeys = new LinkedHashSet<>();
- 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[] topologyKeys = topologyDef.trim().split("/");
+ int lastValidTypeIdx = 0;
+ for (int i = 0; i < topologyKeys.length; i++) {
+ if (topologyKeys[i].length() != 0) {
+ _clusterTopologyKeys.add(topologyKeys[i]);
+ lastValidTypeIdx = i;
}
}
- if (_types.size() == 0) {
- logger.error("Invalid cluster topology definition " + topologyDef);
+ if (_clusterTopologyKeys.size() == 0) {
throw new HelixException("Invalid cluster topology definition " +
topologyDef);
- } else {
- String lastType = null;
- for (String type : _types) {
- _defaultDomainPathValues.put(type, "Helix_default_" + type);
Review comment:
Where do you update this map? I didn't see in your latest version.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -212,123 +185,104 @@ 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;
+ .format("ZONE_ID for instance %s 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(
+ String.format("Domain for instance %s 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.
+ instanceTopologyMap.put(key, domainAsMap.getOrDefault(key,
"Helix_default_" + key));
+ }
}
- 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 true;
}
-
/**
* Add an end node to the tree, create all the paths to the leaf node if not
present.
*/
- private Node addEndNode(Node root, String instanceName, Map<String, String>
pathNameMap,
+ private void addEndNode(Node root, String instanceName,
LinkedHashMap<String, String> pathNameMap,
Review comment:
I talked with Xiaoyuan, since the method logic requires the input map
has a fixed iterate order, I suggest her to use the explicit type. Please note
that if you input a Map instantiate by HashMap, then the method won't work as
expected!
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -212,123 +190,124 @@ 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;
- }
-
+ // TODO: Currently we add disabled instance to the topology tree. Since
they are not considered
+ // TODO: in relabalnce, maybe we should skip adding them to the tree for
consistence.
+ for (String instanceName : _allInstances) {
+ InstanceConfig insConfig = _instanceConfigMap.get(instanceName);
+ LinkedHashMap<String, String> instanceTopologyMap;
+ try {
+ instanceTopologyMap =
+
computeInstanceTopologyMap(_clusterConfig.isTopologyAwareEnabled(),
instanceName,
+ insConfig, _clusterTopologyKeys, _defaultDomainPathValues);
+ int weight = insConfig.getWeight();
+ if (weight < 0 || weight == InstanceConfig.WEIGHT_NOT_SET) {
+ weight = DEFAULT_NODE_WEIGHT;
+ }
+ addEndNode(root, instanceName, instanceTopologyMap, weight,
_liveInstances);
+ } catch (IllegalArgumentException e) {
+ if (isInstanceEnabled(_clusterConfig, instanceName, insConfig)) {
+ throw e;
+ } else {
+ logger
+ .warn("Topology setting {} for instance {} is unset or invalid,
ignore the instance!",
+ insConfig.getDomainAsString(), instanceName);
}
- 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;
}
+ private boolean isInstanceEnabled(ClusterConfig clusterConfig, String
instanceName,
+ InstanceConfig instanceConfig) {
+ return (instanceConfig.getInstanceEnabled() &&
(clusterConfig.getDisabledInstances() == null
+ || !clusterConfig.getDisabledInstances().containsKey(instanceName)));
+ }
+
/**
- * 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.
+ *
+ * @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(boolean
isTopologyAwareEnabled,
+ String instanceName, InstanceConfig instanceConfig,
LinkedHashSet<String> clusterTopologyKeys,
+ Map<String, String> defaultDomainPathValuesCache) throws
IllegalArgumentException {
+ LinkedHashMap<String, String> instanceTopologyMap = new LinkedHashMap<>();
+ if (isTopologyAwareEnabled) {
+ if (clusterTopologyKeys == null || clusterTopologyKeys.size() == 0) {
Review comment:
It won't be null. Don't overprotect the code, it may hide issues.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -212,123 +190,124 @@ 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;
- }
-
+ // TODO: Currently we add disabled instance to the topology tree. Since
they are not considered
+ // TODO: in relabalnce, maybe we should skip adding them to the tree for
consistence.
+ for (String instanceName : _allInstances) {
+ InstanceConfig insConfig = _instanceConfigMap.get(instanceName);
+ LinkedHashMap<String, String> instanceTopologyMap;
+ try {
+ instanceTopologyMap =
+
computeInstanceTopologyMap(_clusterConfig.isTopologyAwareEnabled(),
instanceName,
+ insConfig, _clusterTopologyKeys, _defaultDomainPathValues);
+ int weight = insConfig.getWeight();
+ if (weight < 0 || weight == InstanceConfig.WEIGHT_NOT_SET) {
+ weight = DEFAULT_NODE_WEIGHT;
+ }
+ addEndNode(root, instanceName, instanceTopologyMap, weight,
_liveInstances);
+ } catch (IllegalArgumentException e) {
+ if (isInstanceEnabled(_clusterConfig, instanceName, insConfig)) {
+ throw e;
+ } else {
+ logger
+ .warn("Topology setting {} for instance {} is unset or invalid,
ignore the instance!",
+ insConfig.getDomainAsString(), instanceName);
}
- 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;
}
+ private boolean isInstanceEnabled(ClusterConfig clusterConfig, String
instanceName,
+ InstanceConfig instanceConfig) {
+ return (instanceConfig.getInstanceEnabled() &&
(clusterConfig.getDisabledInstances() == null
+ || !clusterConfig.getDisabledInstances().containsKey(instanceName)));
+ }
+
/**
- * 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.
+ *
+ * @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(boolean
isTopologyAwareEnabled,
+ String instanceName, InstanceConfig instanceConfig,
LinkedHashSet<String> clusterTopologyKeys,
+ Map<String, String> defaultDomainPathValuesCache) throws
IllegalArgumentException {
+ LinkedHashMap<String, String> instanceTopologyMap = new LinkedHashMap<>();
+ if (isTopologyAwareEnabled) {
+ if (clusterTopologyKeys == null || clusterTopologyKeys.size() == 0) {
+ // Return a ordered map using default cluster topology definition,
i,e. /root/zone/instance
+ String zone = instanceConfig.getZoneId();
+ if (zone == null) {
+ throw new IllegalArgumentException(String
+ .format("ZONE_ID for instance %s is not set, fail the
topology-aware placement!",
+ instanceName));
}
- }
-
- 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()) {
- throw new HelixException(String.format(
- "Domain-Value pair %s for instance %s is not valid, failed the
topology-aware placement!",
- pair, ins));
+ instanceTopologyMap.put(Types.ZONE.name(), zone);
+ instanceTopologyMap.put(Types.INSTANCE.name(), instanceName);
+ } else {
+ /*
+ * Return a ordered map representing the instance path. The topology
order is defined in
+ * ClusterConfig.topology.
+ */
+ Map<String, String> domainAsMap;
+ try {
+ domainAsMap = instanceConfig.getDomainAsMap();
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException(String
+ .format("Domain %s for instance %s is not valid, fail the
topology-aware placement!",
+ instanceConfig.getDomainAsString(), instanceName), e);
}
- 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;
+ if (domainAsMap == null || domainAsMap.isEmpty()) {
+ throw new IllegalArgumentException(String
+ .format("Domain for instance %s is not set, fail the
topology-aware placement!",
+ instanceName));
+ }
+ int numOfMatchedKeys = 0;
+ for (String key : clusterTopologyKeys) {
+ // if a key does not exist in the instance domain config, using the
default domain value.
+ String value = domainAsMap.get(key);
+ if (value == null || value.length() == 0) {
+ value =
+ defaultDomainPathValuesCache.computeIfAbsent(key, k ->
(DEFAULT_DOMAIN_PREFIX + key));
Review comment:
I see you want a lazy initialization. It's good design.
But since you are modifying the input parameter implicitly, it might be
clearer to just refer to _defaultDomainPathValues. Otherwise, there is an
implicit assumption that the caller needs to pass the same map for all the
calls.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -212,123 +190,124 @@ 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;
- }
-
+ // TODO: Currently we add disabled instance to the topology tree. Since
they are not considered
+ // TODO: in relabalnce, maybe we should skip adding them to the tree for
consistence.
+ for (String instanceName : _allInstances) {
+ InstanceConfig insConfig = _instanceConfigMap.get(instanceName);
+ LinkedHashMap<String, String> instanceTopologyMap;
+ try {
+ instanceTopologyMap =
+
computeInstanceTopologyMap(_clusterConfig.isTopologyAwareEnabled(),
instanceName,
+ insConfig, _clusterTopologyKeys, _defaultDomainPathValues);
+ int weight = insConfig.getWeight();
+ if (weight < 0 || weight == InstanceConfig.WEIGHT_NOT_SET) {
+ weight = DEFAULT_NODE_WEIGHT;
+ }
+ addEndNode(root, instanceName, instanceTopologyMap, weight,
_liveInstances);
+ } catch (IllegalArgumentException e) {
+ if (isInstanceEnabled(_clusterConfig, instanceName, insConfig)) {
+ throw e;
+ } else {
+ logger
+ .warn("Topology setting {} for instance {} is unset or invalid,
ignore the instance!",
+ insConfig.getDomainAsString(), instanceName);
}
- 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;
}
+ private boolean isInstanceEnabled(ClusterConfig clusterConfig, String
instanceName,
+ InstanceConfig instanceConfig) {
+ return (instanceConfig.getInstanceEnabled() &&
(clusterConfig.getDisabledInstances() == null
+ || !clusterConfig.getDisabledInstances().containsKey(instanceName)));
+ }
+
/**
- * 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.
+ *
+ * @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(boolean
isTopologyAwareEnabled,
+ String instanceName, InstanceConfig instanceConfig,
LinkedHashSet<String> clusterTopologyKeys,
+ Map<String, String> defaultDomainPathValuesCache) throws
IllegalArgumentException {
+ LinkedHashMap<String, String> instanceTopologyMap = new LinkedHashMap<>();
+ if (isTopologyAwareEnabled) {
+ if (clusterTopologyKeys == null || clusterTopologyKeys.size() == 0) {
+ // Return a ordered map using default cluster topology definition,
i,e. /root/zone/instance
+ String zone = instanceConfig.getZoneId();
+ if (zone == null) {
+ throw new IllegalArgumentException(String
+ .format("ZONE_ID for instance %s is not set, fail the
topology-aware placement!",
+ instanceName));
}
- }
-
- 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()) {
- throw new HelixException(String.format(
- "Domain-Value pair %s for instance %s is not valid, failed the
topology-aware placement!",
- pair, ins));
+ instanceTopologyMap.put(Types.ZONE.name(), zone);
+ instanceTopologyMap.put(Types.INSTANCE.name(), instanceName);
+ } else {
+ /*
+ * Return a ordered map representing the instance path. The topology
order is defined in
+ * ClusterConfig.topology.
+ */
+ Map<String, String> domainAsMap;
+ try {
+ domainAsMap = instanceConfig.getDomainAsMap();
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException(String
+ .format("Domain %s for instance %s is not valid, fail the
topology-aware placement!",
+ instanceConfig.getDomainAsString(), instanceName), e);
}
- 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;
+ if (domainAsMap == null || domainAsMap.isEmpty()) {
+ throw new IllegalArgumentException(String
+ .format("Domain for instance %s is not set, fail the
topology-aware placement!",
+ instanceName));
+ }
+ int numOfMatchedKeys = 0;
+ for (String key : clusterTopologyKeys) {
+ // if a key does not exist in the instance domain config, using the
default domain value.
+ String value = domainAsMap.get(key);
+ if (value == null || value.length() == 0) {
+ value =
+ defaultDomainPathValuesCache.computeIfAbsent(key, k ->
(DEFAULT_DOMAIN_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 "
+ + "{}, using default domain value instead",
instanceConfig.getDomainAsString(),
+ clusterTopologyKeys.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(), instanceName);
Review comment:
Add the corresponding comment as you did for the other 2 conditions
above.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -212,123 +190,124 @@ 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;
- }
-
+ // TODO: Currently we add disabled instance to the topology tree. Since
they are not considered
+ // TODO: in relabalnce, maybe we should skip adding them to the tree for
consistence.
+ for (String instanceName : _allInstances) {
+ InstanceConfig insConfig = _instanceConfigMap.get(instanceName);
+ LinkedHashMap<String, String> instanceTopologyMap;
+ try {
+ instanceTopologyMap =
+
computeInstanceTopologyMap(_clusterConfig.isTopologyAwareEnabled(),
instanceName,
+ insConfig, _clusterTopologyKeys, _defaultDomainPathValues);
+ int weight = insConfig.getWeight();
+ if (weight < 0 || weight == InstanceConfig.WEIGHT_NOT_SET) {
+ weight = DEFAULT_NODE_WEIGHT;
+ }
+ addEndNode(root, instanceName, instanceTopologyMap, weight,
_liveInstances);
+ } catch (IllegalArgumentException e) {
+ if (isInstanceEnabled(_clusterConfig, instanceName, insConfig)) {
+ throw e;
+ } else {
+ logger
+ .warn("Topology setting {} for instance {} is unset or invalid,
ignore the instance!",
+ insConfig.getDomainAsString(), instanceName);
}
- 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;
}
+ private boolean isInstanceEnabled(ClusterConfig clusterConfig, String
instanceName,
+ InstanceConfig instanceConfig) {
+ return (instanceConfig.getInstanceEnabled() &&
(clusterConfig.getDisabledInstances() == null
+ || !clusterConfig.getDisabledInstances().containsKey(instanceName)));
+ }
+
/**
- * 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.
+ *
+ * @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(boolean
isTopologyAwareEnabled,
+ String instanceName, InstanceConfig instanceConfig,
LinkedHashSet<String> clusterTopologyKeys,
+ Map<String, String> defaultDomainPathValuesCache) throws
IllegalArgumentException {
+ LinkedHashMap<String, String> instanceTopologyMap = new LinkedHashMap<>();
+ if (isTopologyAwareEnabled) {
+ if (clusterTopologyKeys == null || clusterTopologyKeys.size() == 0) {
+ // Return a ordered map using default cluster topology definition,
i,e. /root/zone/instance
+ String zone = instanceConfig.getZoneId();
+ if (zone == null) {
+ throw new IllegalArgumentException(String
+ .format("ZONE_ID for instance %s is not set, fail the
topology-aware placement!",
+ instanceName));
}
- }
-
- 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()) {
- throw new HelixException(String.format(
- "Domain-Value pair %s for instance %s is not valid, failed the
topology-aware placement!",
- pair, ins));
+ instanceTopologyMap.put(Types.ZONE.name(), zone);
+ instanceTopologyMap.put(Types.INSTANCE.name(), instanceName);
+ } else {
+ /*
+ * Return a ordered map representing the instance path. The topology
order is defined in
+ * ClusterConfig.topology.
+ */
+ Map<String, String> domainAsMap;
+ try {
+ domainAsMap = instanceConfig.getDomainAsMap();
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException(String
+ .format("Domain %s for instance %s is not valid, fail the
topology-aware placement!",
+ instanceConfig.getDomainAsString(), instanceName), e);
}
- 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;
+ if (domainAsMap == null || domainAsMap.isEmpty()) {
Review comment:
It won't be null, I guess. Don't overprotect the code, it may hide
issues.
----------------------------------------------------------------
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]
