xyuanlu commented on a change in pull request #1043:
URL: https://github.com/apache/helix/pull/1043#discussion_r432893951
##########
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) {
Review comment:
GetDomainAsMap will throw IllegalArgumentException if
1. The Splitter.on() param matches the empty string.
2. Has duplicate Key.
For example the controller does not know how to categorize the instance if
user specified two different rack.
There is one behavior change (witch I should have documented in the pr
description) is that, GetDomainAsMap won't throw exception if either key or
value is an empty string. I think the new behavior is reasonable since
1. We ignore keys in DOMAIN that are not defined in ClusterConfig. When we
have empty key, they should be treated as non-defined key. Instead of throw
exception, it is ok to just ignore them. They are no harm to the topology tree
anyway.
2. We will use default values for keys that are defined in ClusterConfig but
missing InstanceConfig.DOMAIN. If there is an empty value defined, we should
use the default value as well.
----------------------------------------------------------------
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]
