You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/05/30 07:49:10 UTC
[GitHub] [helix] pkuwm commented on a change in pull request #1043: Code clean up Topology.java
pkuwm commented on a change in pull request #1043:
URL: https://github.com/apache/helix/pull/1043#discussion_r432818347
##########
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));
Review comment:
Instead of `String.format`, I suggest parameterized logging: `logger.warn("Domain for instance {} is not set, ignore the instance!", instance);`
`String.format` is slower. And parameterized logging eliminates string concatenation if WARN level logging is not enabled.
Same for the other logging you changed in this PR.
##########
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:
In what case the `IllegalArgumentException` is thrown from `getDomainAsMap`? Is this same behavior with the old condition check?
##########
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:
`LinkedHashMap` implements `Map` interface. I suggest keeping `Map`.
##########
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));
Review comment:
I suggest making "Helix_default_" a constant because the constant eliminates redundant string objects: constant is only one string object but this would create one string each time.
##########
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<>();
Review comment:
I suggest using `Map` interface as type.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org