[GitHub] [helix] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337825469
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterContext.java
##
@@ -35,14 +35,14 @@
* This class tracks the rebalance-related global cluster status.
*/
public class ClusterContext {
- private final static float ERROR_MARGIN_FOR_ESTIMATED_MAX_COUNT = 1.1f;
-
// This estimation helps to ensure global partition count evenness
private final int _estimatedMaxPartitionCount;
// This estimation helps to ensure global top state replica count evenness
private final int _estimatedMaxTopStateCount;
// This estimation helps to ensure per-resource partition count evenness
private final Map _estimatedMaxPartitionByResource = new
HashMap<>();
+ // This estmations helps to ensure global resource usage evenness.
+ private final float _estimatedMaxUtilization;
Review comment:
projected might be a better word.
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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337823992
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableNode.java
##
@@ -222,13 +217,21 @@ public int getAssignedReplicaCount() {
/**
* Return the most concerning capacity utilization number for evenly
partition assignment.
* The method dynamically returns the highest utilization number among all
the capacity
- * categories.
+ * categories assuming the new capacity usage is added to the node.
* For example, if the current node usage is {CPU: 0.9, MEM: 0.4, DISK:
0.6}. Then this call shall
* return 0.9.
+ * @param newUsage the proposed new additional capacity usage.
* @return The highest utilization number of the node among all the capacity
category.
*/
- public float getHighestCapacityUtilization() {
-return _highestCapacityUtilization;
+ public float getExpectedHighestUtilization(Map newUsage) {
Review comment:
When I first saw this name, I was confused. I think it would be clearer if
you renamed this function to:
getProjectedHighestUtil... (preferred)
or
getEstimatedHighestUtil...
Is this mainly to reduce duplicate code?
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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337824554
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ResourcePartitionAntiAffinityConstraint.java
##
@@ -29,24 +29,16 @@
* the same resource be assigned to the same node to minimize the impact of
node failure scenarios.
* The score is higher the fewer the partitions are on the node belonging to
the same resource.
*/
-class ResourcePartitionAntiAffinityConstraint extends SoftConstraint {
- private static final float MAX_SCORE = 1f;
- private static final float MIN_SCORE = 0f;
-
- ResourcePartitionAntiAffinityConstraint() {
-super(MAX_SCORE, MIN_SCORE);
- }
+class ResourcePartitionAntiAffinityConstraint extends UsageSoftConstraint {
@Override
protected float getAssignmentScore(AssignableNode node, AssignableReplica
replica,
ClusterContext clusterContext) {
String resource = replica.getResourceName();
int curPartitionCountForResource =
node.getAssignedPartitionsByResource(resource).size();
-int doubleMaxPartitionCountForResource =
-2 * clusterContext.getEstimatedMaxPartitionByResource(resource);
-// The score measures the twice the max allowed count versus current counts
-// The returned value is a measurement of remaining quota ratio, in the
case of exceeding allowed counts, return 0
-return Math.max(((float) doubleMaxPartitionCountForResource -
curPartitionCountForResource)
-/ doubleMaxPartitionCountForResource, 0);
+int estimatedMaxPartitionCountForResource =
+clusterContext.getEstimatedMaxPartitionByResource(resource);
Review comment:
getProjectedMaxPartition...
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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337824224
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##
@@ -54,23 +52,18 @@
@Override
protected float getAssignmentScore(AssignableNode node, AssignableReplica
replica,
ClusterContext clusterContext) {
+// Prioritize the matching of the previous Best Possible assignment.
Map bestPossibleStateMap =
getStateMap(replica, clusterContext.getBestPossibleAssignment());
+if (!bestPossibleStateMap.isEmpty()) {
+ return calculateAssignmentScale(node, replica, bestPossibleStateMap);
+}
Map baselineStateMap =
getStateMap(replica, clusterContext.getBaselineAssignment());
-
-// Prioritize the matching of the previous Best Possible assignment.
-float scale = calculateAssignmentScale(node, replica,
bestPossibleStateMap);
-// If the baseline is also provided, adjust the final score accordingly.
-scale = scale * (1 - BASELINE_MATCH_FACTOR)
-+ calculateAssignmentScale(node, replica, baselineStateMap) *
BASELINE_MATCH_FACTOR;
-
-return scale;
- }
-
- @Override
- NormalizeFunction getNormalizeFunction() {
-return score -> score * (getMaxScore() - getMinScore()) + getMinScore();
+if (!baselineStateMap.isEmpty()) {
+ return calculateAssignmentScale(node, replica, baselineStateMap);
+}
+return 0;
Review comment:
How come we don't use a "smoothing" function 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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337820908
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/InstancePartitionsCountConstraint.java
##
@@ -29,20 +29,13 @@
* Discourage the assignment if the instance's occupancy rate is above average
* The normalized score will be within [0, 1]
*/
-class InstancePartitionsCountConstraint extends SoftConstraint {
- private static final float MAX_SCORE = 1f;
- private static final float MIN_SCORE = 0f;
-
- InstancePartitionsCountConstraint() {
-super(MAX_SCORE, MIN_SCORE);
- }
+class InstancePartitionsCountConstraint extends UsageSoftConstraint {
@Override
protected float getAssignmentScore(AssignableNode node, AssignableReplica
replica,
ClusterContext clusterContext) {
-float doubleEstimatedMaxPartitionCount = 2 *
clusterContext.getEstimatedMaxPartitionCount();
+float estimatedMaxPartitionCount =
clusterContext.getEstimatedMaxPartitionCount();
Review comment:
Q: so we don't have to double it because we increased the weight?
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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337825066
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableNode.java
##
@@ -222,13 +217,21 @@ public int getAssignedReplicaCount() {
/**
* Return the most concerning capacity utilization number for evenly
partition assignment.
* The method dynamically returns the highest utilization number among all
the capacity
Review comment:
returns "the projected, " highest utilization...
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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337825645
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterContext.java
##
@@ -71,9 +74,31 @@
int replicaCnt = Math.max(1, estimateAvgReplicaCount(replicas,
instanceCount));
_estimatedMaxPartitionByResource.put(entry.getKey(), replicaCnt);
- totalTopStateReplicas +=
entry.getValue().stream().filter(AssignableReplica::isReplicaTopState).count();
+ for (AssignableReplica replica : entry.getValue()) {
+if (replica.isReplicaTopState()) {
+ totalTopStateReplicas += 1;
+}
+replica.getCapacity().entrySet().stream().forEach(capacityEntry ->
totalUsage
+.compute(capacityEntry.getKey(),
+(k, v) -> (v == null) ? capacityEntry.getValue() : (v +
capacityEntry.getValue(;
+ }
+}
+nodeSet.stream().forEach(node ->
node.getMaxCapacity().entrySet().stream().forEach(
+capacityEntry -> totalCapacity.compute(capacityEntry.getKey(),
+(k, v) -> (v == null) ? capacityEntry.getValue() : (v +
capacityEntry.getValue();
+
+if (totalCapacity.isEmpty()) {
+ _estimatedMaxUtilization = 1f;
Review comment:
projected
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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues. URL: https://github.com/apache/helix/pull/520#discussion_r337824491 ## File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java ## @@ -44,8 +44,6 @@ // This factor indicates the default score that is evaluated if only partition allocation matches // (states are different). Review comment: Let's revise this description. Confusing - partition allocation matches what? states are different -> do you mean states are not factored in this calculation? 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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337827587
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterContext.java
##
@@ -71,9 +74,31 @@
int replicaCnt = Math.max(1, estimateAvgReplicaCount(replicas,
instanceCount));
_estimatedMaxPartitionByResource.put(entry.getKey(), replicaCnt);
- totalTopStateReplicas +=
entry.getValue().stream().filter(AssignableReplica::isReplicaTopState).count();
+ for (AssignableReplica replica : entry.getValue()) {
+if (replica.isReplicaTopState()) {
+ totalTopStateReplicas += 1;
+}
+replica.getCapacity().entrySet().stream().forEach(capacityEntry ->
totalUsage
+.compute(capacityEntry.getKey(),
+(k, v) -> (v == null) ? capacityEntry.getValue() : (v +
capacityEntry.getValue(;
+ }
+}
+nodeSet.stream().forEach(node ->
node.getMaxCapacity().entrySet().stream().forEach(
+capacityEntry -> totalCapacity.compute(capacityEntry.getKey(),
+(k, v) -> (v == null) ? capacityEntry.getValue() : (v +
capacityEntry.getValue();
+
+if (totalCapacity.isEmpty()) {
+ _estimatedMaxUtilization = 1f;
+} else {
+ float estimatedMaxUsage = 0;
+ for (String capacityKey : totalCapacity.keySet()) {
+int maxCapacity = totalCapacity.get(capacityKey);
+int usage = totalUsage.getOrDefault(capacityKey, 0);
+float utilization = (maxCapacity == 0) ? 1 : (float) usage /
maxCapacity;
+estimatedMaxUsage = Math.max(estimatedMaxUsage, utilization);
+ }
+ _estimatedMaxUtilization = estimatedMaxUsage;
Review comment:
Let's have an offline discussion for this.
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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337825572
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterContext.java
##
@@ -105,6 +130,10 @@ public int getEstimatedMaxTopStateCount() {
return _estimatedMaxTopStateCount;
}
+ public float getEstimatedMaxUtilization() {
Review comment:
Projected
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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337820819
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##
@@ -24,31 +24,25 @@
import java.util.Map;
import java.util.Properties;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Maps;
import org.apache.helix.HelixManagerProperties;
import org.apache.helix.SystemPropertyKeys;
import org.apache.helix.controller.rebalancer.waged.RebalanceAlgorithm;
import org.apache.helix.model.ClusterConfig;
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Maps;
-
/**
* The factory class to create an instance of {@link ConstraintBasedAlgorithm}
*/
public class ConstraintBasedAlgorithmFactory {
- // Evenness constraints tend to score within a smaller range.
- // In order to let their scores cause enough difference in the final
evaluation result, we need to
- // enlarge the overall weight of the evenness constraints compared with the
movement constraint.
- // TODO: Tune or make the following factor configurable.
- private static final int EVENNESS_PREFERENCE_NORMALIZE_FACTOR = 50;
private static final Map MODEL = new HashMap()
{
{
// The default setting
put(PartitionMovementConstraint.class.getSimpleName(), 1f);
- put(InstancePartitionsCountConstraint.class.getSimpleName(), 0.3f);
- put(ResourcePartitionAntiAffinityConstraint.class.getSimpleName(), 0.1f);
- put(ResourceTopStateAntiAffinityConstraint.class.getSimpleName(), 0.1f);
- put(MaxCapacityUsageInstanceConstraint.class.getSimpleName(), 0.5f);
+ put(InstancePartitionsCountConstraint.class.getSimpleName(), 5f);
+ put(ResourcePartitionAntiAffinityConstraint.class.getSimpleName(), 5f);
+ put(ResourceTopStateAntiAffinityConstraint.class.getSimpleName(), 15f);
+ put(MaxCapacityUsageInstanceConstraint.class.getSimpleName(), 30f);
Review comment:
Question: how did you tune this?
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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337824637
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ResourceTopStateAntiAffinityConstraint.java
##
@@ -28,26 +28,17 @@
* The higher number the number of top state partitions assigned to the
instance, the lower the
* score, vice versa.
*/
-class ResourceTopStateAntiAffinityConstraint extends SoftConstraint {
- private static final float MAX_SCORE = 1f;
- private static final float MIN_SCORE = 0f;
-
- ResourceTopStateAntiAffinityConstraint() {
-super(MAX_SCORE, MIN_SCORE);
- }
-
+class ResourceTopStateAntiAffinityConstraint extends UsageSoftConstraint {
@Override
protected float getAssignmentScore(AssignableNode node, AssignableReplica
replica,
ClusterContext clusterContext) {
if (!replica.isReplicaTopState()) {
- return (getMaxScore() + getMinScore()) / 2.0f;
+ return 0;
Review comment:
No smoothing?
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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337825413
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableNode.java
##
@@ -222,13 +217,21 @@ public int getAssignedReplicaCount() {
/**
* Return the most concerning capacity utilization number for evenly
partition assignment.
* The method dynamically returns the highest utilization number among all
the capacity
- * categories.
+ * categories assuming the new capacity usage is added to the node.
* For example, if the current node usage is {CPU: 0.9, MEM: 0.4, DISK:
0.6}. Then this call shall
* return 0.9.
+ * @param newUsage the proposed new additional capacity usage.
* @return The highest utilization number of the node among all the capacity
category.
*/
- public float getHighestCapacityUtilization() {
-return _highestCapacityUtilization;
+ public float getExpectedHighestUtilization(Map newUsage) {
+float highestCapacityUtilization = 0;
+for (String capacityKey : _maxAllowedCapacity.keySet()) {
+ float capacityValue = _maxAllowedCapacity.get(capacityKey);
+ float utilization = (capacityValue - _remainingCapacity.get(capacityKey)
+ newUsage
+ .getOrDefault(capacityKey, 0)) / capacityValue;
Review comment:
gerOrDefault (0.0f) just in case :)
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] narendly commented on a change in pull request #520: Refactoring soft constraints to simply the algorithm and fix potential issues.
narendly commented on a change in pull request #520: Refactoring soft
constraints to simply the algorithm and fix potential issues.
URL: https://github.com/apache/helix/pull/520#discussion_r337826099
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterContext.java
##
@@ -55,12 +55,15 @@
/**
* Construct the cluster context based on the current instance status.
* @param replicaSet All the partition replicas that are managed by the
rebalancer
- * @param instanceCount The count of all the active instances that can be
used to host partitions.
+ * @param nodeSet All the active nodes that are managed by the rebalancer
*/
- ClusterContext(Set replicaSet, int instanceCount,
+ ClusterContext(Set replicaSet, Set
nodeSet,
Map baselineAssignment, Map bestPossibleAssignment) {
+int instanceCount = nodeSet.size();
int totalReplicas = 0;
int totalTopStateReplicas = 0;
+Map totalUsage = new HashMap<>();
+Map totalCapacity = new HashMap<>();
for (Map.Entry> entry : replicaSet.stream()
.collect(Collectors.groupingBy(AssignableReplica::getResourceName))
Review comment:
Do we even need to group by resourceName? How does that affect our mappings?
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
