[GitHub] [helix] jiajunwang commented on a change in pull request #413: Add cluster level default instance config.
jiajunwang commented on a change in pull request #413: Add cluster level
default instance config.
URL: https://github.com/apache/helix/pull/413#discussion_r314883260
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableNode.java
##
@@ -88,11 +88,15 @@ private void refresh(ClusterConfig clusterConfig,
InstanceConfig instanceConfig,
Collection existingAssignment) {
reset();
-_currentCapacity.putAll(instanceConfig.getInstanceCapacityMap());
+Map instanceCapaity =
instanceConfig.getInstanceCapacityMap();
Review comment:
Discussed with Hunter offline. I got the point now. Basically, we can
potentially save the temp variable instanceCapaity with the suggested code.
This is a valid point. But for this refresh logic, I prefer not to set and read
the private field in the same function. Just a way to avoid confusing and
reduce potential bug.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #413: Add cluster level default instance config.
jiajunwang commented on a change in pull request #413: Add cluster level
default instance config.
URL: https://github.com/apache/helix/pull/413#discussion_r314872913
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -200,4 +200,17 @@ public void testParseFaultZone() throws IOException {
Assert.assertEquals(assignableNode.getFaultZone(), "2/testInstance/");
}
+
+ @Test
+ public void testDefaultInstanceCapacity() {
+ClusterConfig testClusterConfig = new ClusterConfig("testClusterConfigId");
+testClusterConfig.setDefaultInstanceCapacityMap(_capacityDataMap);
+
+InstanceConfig testInstanceConfig = new
InstanceConfig("testInstanceConfigId");
+
+AssignableNode assignableNode =
+new AssignableNode(testClusterConfig, testInstanceConfig,
_testInstanceId,
+Collections.emptyList());
+Assert.assertEquals(assignableNode.getMaxCapacity(), _capacityDataMap);
Review comment:
The test for verifying default capacity in the Cluster Config has been done
in the TestClusterConfig.
For the test here, the instance capacity is applied to the assignable node
"maxCapacity" field
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #413: Add cluster level default instance config.
jiajunwang commented on a change in pull request #413: Add cluster level
default instance config.
URL: https://github.com/apache/helix/pull/413#discussion_r314872913
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -200,4 +200,17 @@ public void testParseFaultZone() throws IOException {
Assert.assertEquals(assignableNode.getFaultZone(), "2/testInstance/");
}
+
+ @Test
+ public void testDefaultInstanceCapacity() {
+ClusterConfig testClusterConfig = new ClusterConfig("testClusterConfigId");
+testClusterConfig.setDefaultInstanceCapacityMap(_capacityDataMap);
+
+InstanceConfig testInstanceConfig = new
InstanceConfig("testInstanceConfigId");
+
+AssignableNode assignableNode =
+new AssignableNode(testClusterConfig, testInstanceConfig,
_testInstanceId,
+Collections.emptyList());
+Assert.assertEquals(assignableNode.getMaxCapacity(), _capacityDataMap);
Review comment:
The capacity is applied to "max capacity", that is also true when the
default capacity is used.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #413: Add cluster level default instance config.
jiajunwang commented on a change in pull request #413: Add cluster level
default instance config.
URL: https://github.com/apache/helix/pull/413#discussion_r314872555
##
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##
@@ -700,6 +703,48 @@ public void setInstanceCapacityKeys(List
capacityKeys) {
return capacityKeys;
}
+ /**
+ * Get the default instance capacity information from the map fields.
+ *
+ * @return data map if it exists, or empty map
+ */
+ public Map getDefaultInstanceCapacityMap() {
+Map capacityData =
+
_record.getMapField(ClusterConfigProperty.DEFAULT_INSTANCE_CAPACITY_MAP.name());
+
+if (capacityData != null) {
+ return capacityData.entrySet().stream().collect(
+ Collectors.toMap(entry -> entry.getKey(), entry ->
Integer.parseInt(entry.getValue(;
+}
+return Collections.emptyMap();
+ }
+
+ /**
+ * Set the default instance capacity information with an Integer mapping
+ *
+ * @param capacityDataMap - map of instance capacity data
+ * @throws IllegalArgumentException - when any of the data value is a
negative number or when the map is empty
+ */
+ public void setDefaultInstanceCapacityMap(Map
capacityDataMap)
+ throws IllegalArgumentException {
+if (capacityDataMap == null || capacityDataMap.size() == 0) {
+ throw new IllegalArgumentException("Default Instance Capacity Data is
empty");
+}
+
+Map capacityData = new HashMap<>();
+
+capacityDataMap.entrySet().stream().forEach(entry -> {
Review comment:
It's not making much difference here.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #413: Add cluster level default instance config.
jiajunwang commented on a change in pull request #413: Add cluster level
default instance config.
URL: https://github.com/apache/helix/pull/413#discussion_r314871440
##
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##
@@ -700,6 +703,48 @@ public void setInstanceCapacityKeys(List
capacityKeys) {
return capacityKeys;
}
+ /**
+ * Get the default instance capacity information from the map fields.
+ *
Review comment:
Yes...
I guess you don't like the extra line? I personally like it. But if you
modify our format file, I can follow that style. No strong preference.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #413: Add cluster level default instance config.
jiajunwang commented on a change in pull request #413: Add cluster level
default instance config.
URL: https://github.com/apache/helix/pull/413#discussion_r314870911
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableNode.java
##
@@ -88,11 +88,15 @@ private void refresh(ClusterConfig clusterConfig,
InstanceConfig instanceConfig,
Collection existingAssignment) {
reset();
-_currentCapacity.putAll(instanceConfig.getInstanceCapacityMap());
+Map instanceCapaity =
instanceConfig.getInstanceCapacityMap();
+if (instanceCapaity.isEmpty()) {
+ instanceCapaity = clusterConfig.getDefaultInstanceCapacityMap();
+}
+_currentCapacity.putAll(instanceCapaity);
_faultZone = computeFaultZone(clusterConfig, instanceConfig);
_instanceTags = new HashSet<>(instanceConfig.getTags());
_disabledPartitionsMap = instanceConfig.getDisabledPartitionsMap();
-_maxCapacity = instanceConfig.getInstanceCapacityMap();
+_maxCapacity = instanceCapaity;
Review comment:
Good catch!
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #413: Add cluster level default instance config.
jiajunwang commented on a change in pull request #413: Add cluster level
default instance config.
URL: https://github.com/apache/helix/pull/413#discussion_r314870372
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableNode.java
##
@@ -88,11 +88,15 @@ private void refresh(ClusterConfig clusterConfig,
InstanceConfig instanceConfig,
Collection existingAssignment) {
reset();
-_currentCapacity.putAll(instanceConfig.getInstanceCapacityMap());
+Map instanceCapaity =
instanceConfig.getInstanceCapacityMap();
Review comment:
getInstanceCapacityMap won't return null. This is guaranteed and the unit
test covers it.
You are right. That one of the reasons why I tried to decouple the model
classes as far as I can. In this case, if one instance is not changed, we can
just keep it. That can be done in the Cluster Model Provder.
It is in my plan, but will be future.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
