xyuanlu commented on a change in pull request #1043:
URL: https://github.com/apache/helix/pull/1043#discussion_r442400605
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -212,123 +191,122 @@ 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);
+ try {
+ LinkedHashMap<String, String> instanceTopologyMap =
+
computeInstanceTopologyMap(_clusterConfig.isTopologyAwareEnabled(),
instanceName,
+ insConfig, _clusterTopologyKeys);
+ 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)
+ throws IllegalArgumentException {
+ LinkedHashMap<String, String> instanceTopologyMap = new LinkedHashMap<>();
+ if (isTopologyAwareEnabled) {
+ if (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 = new HashMap<>();
Review comment:
I kept the original logic (before code clean up) here. One difference is
that getDomainAsMap wont throw exception if key or value is empty.
----------------------------------------------------------------
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]
