[GitHub] [helix] xyuanlu commented on a change in pull request #1078: Do not ignore the baseline assignment when evaluating in PartitionMovementConstraint.

[GitBox <gi...@apache.org>]

xyuanlu commented on a change in pull request #1078:
URL: https://github.com/apache/helix/pull/1078#discussion_r440522242



##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -77,15 +99,16 @@ protected double getAssignmentScore(AssignableNode node, AssignableReplica repli
     return assignment.get(resourceName).getReplicaMap(new Partition(partitionName));
   }
 
-  private double calculateAssignmentScale(AssignableNode node, AssignableReplica replica,
+  private double calculateAssignmentScore(String nodeName, String state,
       Map<String, String> instanceToStateMap) {
-    String instanceName = node.getInstanceName();
-    if (!instanceToStateMap.containsKey(instanceName)) {
-      return 0;
-    } else {
-      return (instanceToStateMap.get(instanceName).equals(replica.getReplicaState()) ? 1 :
-          ALLOCATION_MATCH_FACTOR);
+    if (instanceToStateMap.containsKey(nodeName)) {
+      return state.equals(instanceToStateMap.get(nodeName)) ?
+          // if state matches, no state transition required for the proposed assignment
+          1 :

Review comment:
       Maybe trailing Comments?

##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -40,31 +40,53 @@
 class PartitionMovementConstraint extends SoftConstraint {
   private static final double MAX_SCORE = 1f;
   private static final double MIN_SCORE = 0f;
-  //TODO: these factors will be tuned based on user's preference
-  // This factor indicates the default score that is evaluated if only partition allocation matches
-  // (states are different).
-  private static final double ALLOCATION_MATCH_FACTOR = 0.5;
+  // The scale factor to adjust score when the proposed allocation partially matches the assignment
+  // plan but will require a state transition (with partition movement).
+  // TODO: these factors will be tuned based on user's preference
+  private static final double STATE_TRANSITION_COST_FACTOR = 0.5;
+  private static final double MOVEMENT_COST_FACTOR = 0.25;
 
   PartitionMovementConstraint() {
     super(MAX_SCORE, MIN_SCORE);
   }
 
+  /**
+   * @return 1 if the proposed assignment completely matches the previous best possible assignment
+   *         (or baseline assignment if the replica is newly added).
+   *         STATE_TRANSITION_COST_FACTOR if the proposed assignment's allocation matches the
+   *         previous Best Possible assignment (or baseline assignment if the replica is newly
+   *         added) but state does not match.
+   *         MOVEMENT_COST_FACTOR if the proposed assignment's allocation matches the baseline
+   *         assignment only, but not matches the previous best possible assignment.
+   *         0 if the proposed assignment is a pure random movement.
+   */
   @Override
   protected double getAssignmentScore(AssignableNode node, AssignableReplica replica,
       ClusterContext clusterContext) {
-    // Prioritize the previous Best Possible assignment
     Map<String, String> bestPossibleAssignment =
         getStateMap(replica, clusterContext.getBestPossibleAssignment());
-    if (!bestPossibleAssignment.isEmpty()) {
-      return calculateAssignmentScale(node, replica, bestPossibleAssignment);
-    }
-    // else, compare the baseline only if the best possible assignment does not contain the replica
     Map<String, String> baselineAssignment =
         getStateMap(replica, clusterContext.getBaselineAssignment());
-    if (!baselineAssignment.isEmpty()) {
-      return calculateAssignmentScale(node, replica, baselineAssignment);
+    String nodeName = node.getInstanceName();
+    String state = replica.getReplicaState();
+
+    if (bestPossibleAssignment.isEmpty()) {
+      // If bestPossibleAssignment of the replica is empty, indicating this is a new replica.

Review comment:
       Maybe  block Comments 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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org