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 2021/02/26 23:46:37 UTC
[GitHub] [helix] NealSun96 opened a new pull request #1658: New PartitionMovementConstraint w/ Configurable Influence Factors
NealSun96 opened a new pull request #1658:
URL: https://github.com/apache/helix/pull/1658
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
(Write a concise description including what, why, how)
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- The following is the result of the "mvn test" command on the appropriate module:
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE 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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r587944754
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -171,6 +164,15 @@ public void testSetRebalancePreferenceInvalidNumber() {
testConfig.setGlobalRebalancePreference(preference);
}
+ @Test(expectedExceptions = IllegalArgumentException.class)
+ public void testSetRebalancePreferenceMissingKey() {
+ Map<ClusterConfig.GlobalRebalancePreferenceKey, Integer> preference = new HashMap<>();
+ preference.put(EVENNESS, 1);
+
+ ClusterConfig testConfig = new ClusterConfig("testId");
+ testConfig.setGlobalRebalancePreference(preference);
Review comment:
The original logic will still work though. Just the setup is kind of ignored. I agree that we can make it clearer. But please document this cleanly in the ClusterConfig class.
The concern for me is that if an existing cluster has been configured with a single number, this new version will fail the rebalance in that cluster. There is no alerting to the user except for the failure rebalancer in that 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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585943289
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -122,13 +122,6 @@ public void testGetRebalancePreferenceDefault() {
ClusterConfig testConfig = new ClusterConfig("testId");
Assert.assertEquals(testConfig.getGlobalRebalancePreference(),
ClusterConfig.DEFAULT_GLOBAL_REBALANCE_PREFERENCE);
-
- Map<ClusterConfig.GlobalRebalancePreferenceKey, Integer> preference = new HashMap<>();
Review comment:
On a second thought, do you think it's actually better to move the validation logic in ConstraintBasedAlgorithmFactory? My reasoning for the current approach is that we shouldn't make a special case out of certain keys because I want to keep the solution generic, but doing the check in ConstraintBasedAlgorithmFactory is just delaying the same thing.
It might be better to keep the validation in setter since it's not silent.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585940332
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -122,13 +122,6 @@ public void testGetRebalancePreferenceDefault() {
ClusterConfig testConfig = new ClusterConfig("testId");
Assert.assertEquals(testConfig.getGlobalRebalancePreference(),
ClusterConfig.DEFAULT_GLOBAL_REBALANCE_PREFERENCE);
-
- Map<ClusterConfig.GlobalRebalancePreferenceKey, Integer> preference = new HashMap<>();
Review comment:
They would be able to successfully set the preference, which is already tested by the above test case.
I can add a test case in ConstraintBasedAlgorithmFactory 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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585956187
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -19,48 +19,20 @@
* under the License.
*/
-import java.util.Collections;
import java.util.Map;
import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
-import org.apache.helix.model.Partition;
-import org.apache.helix.model.ResourceAssignment;
+
/**
- * Evaluate the proposed assignment according to the potential partition movements cost.
- * The cost is evaluated based on the difference between the old assignment and the new assignment.
- * In detail, we consider the following two previous assignments as the base.
- * - Baseline assignment that is calculated regardless of the node state (online/offline).
- * - Previous Best Possible assignment.
- * Any change to these two assignments will increase the partition movements cost, so that the
- * evaluated score will become lower.
+ * Evaluate the proposed assignment according to the potential partition movements cost based on
+ * the previous best possible assignment.
+ * The previous best possible assignment is the sole reference, and if it's missing, use baseline
+ * assignment instead.
*/
-class PartitionMovementConstraint extends SoftConstraint {
- private static final double MAX_SCORE = 1f;
- private static final double MIN_SCORE = 0f;
- // 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);
- }
-
- /**
Review comment:
I meant the line 54 part. Then it is OK. Thanks for confiming.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586001298
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##########
@@ -61,21 +61,34 @@ public static RebalanceAlgorithm getInstance(
new ReplicaActivateConstraint(), new NodeMaxPartitionLimitConstraint(),
new ValidGroupTagConstraint(), new SamePartitionOnInstanceConstraint());
- int evennessPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS, 1);
- int movementPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT, 1);
+ int evennessPreference = 1;
+ int movementPreference = 1;
+ if (preferences.containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS) && preferences
+ .containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS)) {
+ evennessPreference = preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS);
+ movementPreference =
+ preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT);
+ }
+
+ boolean forceBaselineConverge = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.FORCE_BASELINE_CONVERGE, 0)
+ > 0;
+ if (forceBaselineConverge) {
+ MODEL.put(PartitionMovementConstraint.class.getSimpleName(), 0.001f);
Review comment:
Let's discuss offline, what you want to do is 2 features instead of one. This could be concerning. Since this forceBaselineConverge option is not exactly what it claims to be.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r588694158
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -171,6 +164,15 @@ public void testSetRebalancePreferenceInvalidNumber() {
testConfig.setGlobalRebalancePreference(preference);
}
+ @Test(expectedExceptions = IllegalArgumentException.class)
+ public void testSetRebalancePreferenceMissingKey() {
+ Map<ClusterConfig.GlobalRebalancePreferenceKey, Integer> preference = new HashMap<>();
+ preference.put(EVENNESS, 1);
+
+ ClusterConfig testConfig = new ClusterConfig("testId");
+ testConfig.setGlobalRebalancePreference(preference);
Review comment:
The old check is added again for backward compatibility. Documents will be updated.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586017382
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##########
@@ -61,21 +61,34 @@ public static RebalanceAlgorithm getInstance(
new ReplicaActivateConstraint(), new NodeMaxPartitionLimitConstraint(),
new ValidGroupTagConstraint(), new SamePartitionOnInstanceConstraint());
- int evennessPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS, 1);
- int movementPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT, 1);
+ int evennessPreference = 1;
+ int movementPreference = 1;
+ if (preferences.containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS) && preferences
+ .containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS)) {
+ evennessPreference = preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS);
+ movementPreference =
+ preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT);
+ }
+
+ boolean forceBaselineConverge = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.FORCE_BASELINE_CONVERGE, 0)
+ > 0;
+ if (forceBaselineConverge) {
+ MODEL.put(PartitionMovementConstraint.class.getSimpleName(), 0.001f);
Review comment:
That's exactly my concern. Let's discuss.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585935700
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -19,48 +19,20 @@
* under the License.
*/
-import java.util.Collections;
import java.util.Map;
import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
-import org.apache.helix.model.Partition;
-import org.apache.helix.model.ResourceAssignment;
+
/**
- * Evaluate the proposed assignment according to the potential partition movements cost.
- * The cost is evaluated based on the difference between the old assignment and the new assignment.
- * In detail, we consider the following two previous assignments as the base.
- * - Baseline assignment that is calculated regardless of the node state (online/offline).
- * - Previous Best Possible assignment.
- * Any change to these two assignments will increase the partition movements cost, so that the
- * evaluated score will become lower.
+ * Evaluate the proposed assignment according to the potential partition movements cost based on
+ * the previous best possible assignment.
+ * The previous best possible assignment is the sole reference, and if it's missing, use baseline
+ * assignment instead.
*/
-class PartitionMovementConstraint extends SoftConstraint {
- private static final double MAX_SCORE = 1f;
- private static final double MIN_SCORE = 0f;
- // 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);
- }
-
- /**
Review comment:
Which part are you referring to? If you mean line 43, it has been moved to `AbstractPartitionMovementConstraint` above the `getStateTransitionCostFactor` function (that is getting renamed). If you're referring to line 54, it has also been moved to `AbstractPartitionMovementConstraint` above `getAssignmentScore`.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585938770
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -160,7 +161,8 @@
DEFAULT_GLOBAL_REBALANCE_PREFERENCE =
Review comment:
This default map is still being used, and the current behavior is exactly as you have described. Looks to me there's nothing to change 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
[GitHub] [helix] jiajunwang commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585086138
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -73,19 +97,13 @@ protected double getAssignmentScore(AssignableNode node, AssignableReplica repli
if (bestPossibleAssignment.isEmpty()) {
// If bestPossibleAssignment of the replica is empty, indicating this is a new replica.
// Then the baseline is the only reference.
- return calculateAssignmentScore(nodeName, state, baselineAssignment);
+ return calculateAssignmentScore(nodeName, state, baselineAssignment)
Review comment:
This code cannot explain itself. The input is baselineAssignment, but you are multiplying it with best possible influence factor.
Also, does it mean we will return 1.25 as the max value instead of 1? This changes the behavior and breaks the assumption. The max value should be 1 so the result can be scaled correctly.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##########
@@ -61,21 +60,29 @@ public static RebalanceAlgorithm getInstance(
new ReplicaActivateConstraint(), new NodeMaxPartitionLimitConstraint(),
new ValidGroupTagConstraint(), new SamePartitionOnInstanceConstraint());
- int evennessPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS, 1);
- int movementPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT, 1);
+ int evennessPreference = 1;
+ int movementPreference = 1;
+ if (preferences.containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS) && preferences
+ .containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS)) {
Review comment:
Two EVENNESS, typo?
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -122,13 +122,6 @@ public void testGetRebalancePreferenceDefault() {
ClusterConfig testConfig = new ClusterConfig("testId");
Assert.assertEquals(testConfig.getGlobalRebalancePreference(),
ClusterConfig.DEFAULT_GLOBAL_REBALANCE_PREFERENCE);
-
- Map<ClusterConfig.GlobalRebalancePreferenceKey, Integer> preference = new HashMap<>();
Review comment:
What would be the expected preference in this condition then?
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -160,7 +161,8 @@
DEFAULT_GLOBAL_REBALANCE_PREFERENCE =
Review comment:
I think the change does make sense. But is this value still being used anymore?
Maybe we can change the logic to, if nothing is configured, then apply this default value.
If partially missing, then put whatever user set in the ClusterConfig but leave the processing logic to the rebalancer like what you are doing right now.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/BaselineInfluenceConstraint.java
##########
@@ -0,0 +1,54 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Map;
+
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+
+
+/**
+ * Evaluate the proposed assignment according to the potential partition movements cost based on
+ * the baseline assignment's influence.
+ * This constraint promotes movements for evenness. If best possible doesn't exist, baseline will be
+ * used to restrict movements, so this constraint should give no score in that case.
+ */
+public class BaselineInfluenceConstraint extends AbstractPartitionMovementConstraint {
+ @Override
+ protected double getAssignmentScore(AssignableNode node, AssignableReplica replica,
+ ClusterContext clusterContext) {
+ Map<String, String> bestPossibleAssignment =
+ getStateMap(replica, clusterContext.getBestPossibleAssignment());
+ Map<String, String> baselineAssignment =
Review comment:
nit, put this section after the bestPossibleAssignment check to avoid unnecessary calculation.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##########
@@ -61,21 +61,34 @@ public static RebalanceAlgorithm getInstance(
new ReplicaActivateConstraint(), new NodeMaxPartitionLimitConstraint(),
new ValidGroupTagConstraint(), new SamePartitionOnInstanceConstraint());
- int evennessPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS, 1);
- int movementPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT, 1);
+ int evennessPreference = 1;
Review comment:
Refer to the value in DEFAULT_GLOBAL_REBALANCE_PREFERENCE?
Please avoid hardcode.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -19,48 +19,20 @@
* under the License.
*/
-import java.util.Collections;
import java.util.Map;
import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
-import org.apache.helix.model.Partition;
-import org.apache.helix.model.ResourceAssignment;
+
/**
- * Evaluate the proposed assignment according to the potential partition movements cost.
- * The cost is evaluated based on the difference between the old assignment and the new assignment.
- * In detail, we consider the following two previous assignments as the base.
- * - Baseline assignment that is calculated regardless of the node state (online/offline).
- * - Previous Best Possible assignment.
- * Any change to these two assignments will increase the partition movements cost, so that the
- * evaluated score will become lower.
+ * Evaluate the proposed assignment according to the potential partition movements cost based on
+ * the previous best possible assignment.
+ * The previous best possible assignment is the sole reference, and if it's missing, use baseline
+ * assignment instead.
*/
-class PartitionMovementConstraint extends SoftConstraint {
- private static final double MAX_SCORE = 1f;
- private static final double MIN_SCORE = 0f;
- // 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
Review comment:
Why remove?
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/AbstractPartitionMovementConstraint.java
##########
@@ -0,0 +1,86 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Collections;
+import java.util.Map;
+
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.ResourceAssignment;
+
+/**
+ * Evaluate the proposed assignment according to the potential partition movements cost.
+ * The cost is evaluated based on the difference between the old assignment and the new assignment.
+ * Any change from the old assignment will increase the partition movements cost, so that the
+ * evaluated score will become lower.
+ */
+abstract class AbstractPartitionMovementConstraint extends SoftConstraint {
+ protected static final double MAX_SCORE = 1f;
+ protected static final double MIN_SCORE = 0f;
+
+ AbstractPartitionMovementConstraint() {
+ super(MAX_SCORE, MIN_SCORE);
+ }
+
+ /**
+ * @return MAX_SCORE if the proposed assignment completely matches the previous assignment.
+ * StateTransitionCostFactor if the proposed assignment's allocation matches the
+ * previous assignment but state does not match.
+ * MIN_SCORE if the proposed assignment completely doesn't match the previous one.
+ */
+ @Override
+ protected abstract double getAssignmentScore(AssignableNode node, AssignableReplica replica,
+ ClusterContext clusterContext);
+
+ /**
+ * @return The scale factor to adjust score when the proposed allocation partially matches the
+ * assignment plan but will require a state transition (with partition movement).
+ */
+ protected abstract double getStateTransitionCostFactor();
Review comment:
It seems that both of the child classes have the same implementation. We don't need this abstract method now.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/BaselineInfluenceConstraint.java
##########
@@ -0,0 +1,54 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Map;
+
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+
+
+/**
+ * Evaluate the proposed assignment according to the potential partition movements cost based on
+ * the baseline assignment's influence.
+ * This constraint promotes movements for evenness. If best possible doesn't exist, baseline will be
+ * used to restrict movements, so this constraint should give no score in that case.
+ */
+public class BaselineInfluenceConstraint extends AbstractPartitionMovementConstraint {
+ @Override
+ protected double getAssignmentScore(AssignableNode node, AssignableReplica replica,
+ ClusterContext clusterContext) {
+ Map<String, String> bestPossibleAssignment =
+ getStateMap(replica, clusterContext.getBestPossibleAssignment());
+ Map<String, String> baselineAssignment =
+ getStateMap(replica, clusterContext.getBaselineAssignment());
+ if (bestPossibleAssignment.isEmpty()) {
+ return getMinScore();
+ }
+ return calculateAssignmentScore(node.getInstanceName(), replica.getReplicaState(),
+ baselineAssignment);
+ }
+
+ @Override
+ protected double getStateTransitionCostFactor() {
+ return MAX_SCORE / 2;
Review comment:
We shall return STATE_TRANSITION_COST_FACTOR here.
1. it should return factor not the actual score.
2. it shall not be hardcoded.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -19,48 +19,20 @@
* under the License.
*/
-import java.util.Collections;
import java.util.Map;
import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
-import org.apache.helix.model.Partition;
-import org.apache.helix.model.ResourceAssignment;
+
/**
- * Evaluate the proposed assignment according to the potential partition movements cost.
- * The cost is evaluated based on the difference between the old assignment and the new assignment.
- * In detail, we consider the following two previous assignments as the base.
- * - Baseline assignment that is calculated regardless of the node state (online/offline).
- * - Previous Best Possible assignment.
- * Any change to these two assignments will increase the partition movements cost, so that the
- * evaluated score will become lower.
+ * Evaluate the proposed assignment according to the potential partition movements cost based on
+ * the previous best possible assignment.
+ * The previous best possible assignment is the sole reference, and if it's missing, use baseline
+ * assignment instead.
Review comment:
"if it's missing, use baseline assignment instead.", this part deserves more description. At least mention that we are doing so because it is assumed to be a new resource that has never been allocated before.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##########
@@ -61,21 +61,34 @@ public static RebalanceAlgorithm getInstance(
new ReplicaActivateConstraint(), new NodeMaxPartitionLimitConstraint(),
new ValidGroupTagConstraint(), new SamePartitionOnInstanceConstraint());
- int evennessPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS, 1);
- int movementPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT, 1);
+ int evennessPreference = 1;
+ int movementPreference = 1;
+ if (preferences.containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS) && preferences
+ .containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS)) {
+ evennessPreference = preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS);
+ movementPreference =
+ preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT);
+ }
+
+ boolean forceBaselineConverge = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.FORCE_BASELINE_CONVERGE, 0)
+ > 0;
+ if (forceBaselineConverge) {
+ MODEL.put(PartitionMovementConstraint.class.getSimpleName(), 0.001f);
Review comment:
And I guess set its weight to (10000 * PartitionMovementConstraint weight * LESS_MOVEMENT preference factor) should be enough and 0.001f maybe not required.
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -122,13 +122,6 @@ public void testGetRebalancePreferenceDefault() {
ClusterConfig testConfig = new ClusterConfig("testId");
Assert.assertEquals(testConfig.getGlobalRebalancePreference(),
ClusterConfig.DEFAULT_GLOBAL_REBALANCE_PREFERENCE);
-
- Map<ClusterConfig.GlobalRebalancePreferenceKey, Integer> preference = new HashMap<>();
Review comment:
Moreover, if we move the auto-fill logic to ConstraintBasedAlgorithmFactory.java, then we need to add a test case there to cover the change.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##########
@@ -61,21 +61,34 @@ public static RebalanceAlgorithm getInstance(
new ReplicaActivateConstraint(), new NodeMaxPartitionLimitConstraint(),
new ValidGroupTagConstraint(), new SamePartitionOnInstanceConstraint());
- int evennessPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS, 1);
- int movementPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT, 1);
+ int evennessPreference = 1;
+ int movementPreference = 1;
+ if (preferences.containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS) && preferences
+ .containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS)) {
+ evennessPreference = preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS);
+ movementPreference =
+ preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT);
+ }
+
+ boolean forceBaselineConverge = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.FORCE_BASELINE_CONVERGE, 0)
+ > 0;
+ if (forceBaselineConverge) {
+ MODEL.put(PartitionMovementConstraint.class.getSimpleName(), 0.001f);
+ MODEL.put(BaselineInfluenceConstraint.class.getSimpleName(), 10000f);
+ }
List<SoftConstraint> softConstraints = ImmutableList
- .of(new PartitionMovementConstraint(), new InstancePartitionsCountConstraint(),
- new ResourcePartitionAntiAffinityConstraint(),
+ .of(new PartitionMovementConstraint(), new BaselineInfluenceConstraint(),
+ new InstancePartitionsCountConstraint(), new ResourcePartitionAntiAffinityConstraint(),
new ResourceTopStateAntiAffinityConstraint(), new MaxCapacityUsageInstanceConstraint());
- Map<SoftConstraint, Float> softConstraintsWithWeight = Maps.toMap(softConstraints, key -> {
- String name = key.getClass().getSimpleName();
- float weight = MODEL.get(name);
- return name.equals(PartitionMovementConstraint.class.getSimpleName()) ?
- movementPreference * weight : evennessPreference * weight;
- });
+ Map<SoftConstraint, Float> softConstraintsWithWeight = new HashMap<>();
+ for (SoftConstraint softConstraint : softConstraints) {
+ float constraintWeight = MODEL.get(softConstraint.getClass().getSimpleName());
+ softConstraintsWithWeight.put(softConstraint,
Review comment:
Comment on why we treat BaselineInfluenceConstraint as an evennessPreference instead of movementPreference please.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##########
@@ -61,21 +61,34 @@ public static RebalanceAlgorithm getInstance(
new ReplicaActivateConstraint(), new NodeMaxPartitionLimitConstraint(),
new ValidGroupTagConstraint(), new SamePartitionOnInstanceConstraint());
- int evennessPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS, 1);
- int movementPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT, 1);
+ int evennessPreference = 1;
+ int movementPreference = 1;
+ if (preferences.containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS) && preferences
+ .containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS)) {
+ evennessPreference = preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS);
+ movementPreference =
+ preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT);
+ }
+
+ boolean forceBaselineConverge = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.FORCE_BASELINE_CONVERGE, 0)
+ > 0;
+ if (forceBaselineConverge) {
+ MODEL.put(PartitionMovementConstraint.class.getSimpleName(), 0.001f);
Review comment:
I think it would be better not to touch this weight. Unlike BaselineInfluenceConstraint, PartitionMovementConstraint is used in the baseline calculation too. So if you modify the weight here, then the baseline will be over evened. This may cause a large partition shuffling.
What we can do should be modifying BaselineInfluenceConstraint **after** the preference has been applied to the the constraint weight.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##########
@@ -39,6 +38,7 @@
{
// The default setting
put(PartitionMovementConstraint.class.getSimpleName(), 2f);
+ put(BaselineInfluenceConstraint.class.getSimpleName(), 0.5f);
put(InstancePartitionsCountConstraint.class.getSimpleName(), 1f);
put(ResourcePartitionAntiAffinityConstraint.class.getSimpleName(), 1f);
put(ResourceTopStateAntiAffinityConstraint.class.getSimpleName(), 3f);
Review comment:
nit, could you please merge the most recent code about top state usage constraint then we can review the updated code.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -71,48 +43,13 @@ protected double getAssignmentScore(AssignableNode node, AssignableReplica repli
String state = replica.getReplicaState();
if (bestPossibleAssignment.isEmpty()) {
- // If bestPossibleAssignment of the replica is empty, indicating this is a new replica.
Review comment:
I think we should keep this comment. Or it would be hard to explain why we are using baselineAssignment as the input here.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/AbstractPartitionMovementConstraint.java
##########
@@ -0,0 +1,86 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Collections;
+import java.util.Map;
+
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.ResourceAssignment;
+
+/**
+ * Evaluate the proposed assignment according to the potential partition movements cost.
+ * The cost is evaluated based on the difference between the old assignment and the new assignment.
+ * Any change from the old assignment will increase the partition movements cost, so that the
+ * evaluated score will become lower.
+ */
+abstract class AbstractPartitionMovementConstraint extends SoftConstraint {
+ protected static final double MAX_SCORE = 1f;
+ protected static final double MIN_SCORE = 0f;
+
+ AbstractPartitionMovementConstraint() {
+ super(MAX_SCORE, MIN_SCORE);
+ }
+
+ /**
+ * @return MAX_SCORE if the proposed assignment completely matches the previous assignment.
+ * StateTransitionCostFactor if the proposed assignment's allocation matches the
+ * previous assignment but state does not match.
+ * MIN_SCORE if the proposed assignment completely doesn't match the previous one.
+ */
+ @Override
+ protected abstract double getAssignmentScore(AssignableNode node, AssignableReplica replica,
+ ClusterContext clusterContext);
+
+ /**
+ * @return The scale factor to adjust score when the proposed allocation partially matches the
+ * assignment plan but will require a state transition (with partition movement).
+ */
+ protected abstract double getStateTransitionCostFactor();
+
+ protected Map<String, String> getStateMap(AssignableReplica replica,
+ Map<String, ResourceAssignment> assignment) {
+ String resourceName = replica.getResourceName();
+ String partitionName = replica.getPartitionName();
+ if (assignment == null || !assignment.containsKey(resourceName)) {
+ return Collections.emptyMap();
+ }
+ return assignment.get(resourceName).getReplicaMap(new Partition(partitionName));
+ }
+
+ protected double calculateAssignmentScore(String nodeName, String state,
+ Map<String, String> instanceToStateMap) {
+ if (instanceToStateMap.containsKey(nodeName)) {
+ return state.equals(instanceToStateMap.get(nodeName)) ?
+ MAX_SCORE : // if state matches, no state transition required for the proposed assignment
+ getStateTransitionCostFactor(); // if state does not match, then the proposed assignment
Review comment:
Should be (MAX_SCORE - MIN_SCORE) * getStateTransitionCostFactor() + MIN_SCORE?
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -19,48 +19,20 @@
* under the License.
*/
-import java.util.Collections;
import java.util.Map;
import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
-import org.apache.helix.model.Partition;
-import org.apache.helix.model.ResourceAssignment;
+
/**
- * Evaluate the proposed assignment according to the potential partition movements cost.
- * The cost is evaluated based on the difference between the old assignment and the new assignment.
- * In detail, we consider the following two previous assignments as the base.
- * - Baseline assignment that is calculated regardless of the node state (online/offline).
- * - Previous Best Possible assignment.
- * Any change to these two assignments will increase the partition movements cost, so that the
- * evaluated score will become lower.
+ * Evaluate the proposed assignment according to the potential partition movements cost based on
+ * the previous best possible assignment.
+ * The previous best possible assignment is the sole reference, and if it's missing, use baseline
+ * assignment instead.
*/
-class PartitionMovementConstraint extends SoftConstraint {
- private static final double MAX_SCORE = 1f;
- private static final double MIN_SCORE = 0f;
- // 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);
- }
-
- /**
Review comment:
Please refine the comment instead of removing it.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586003393
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/BaselineInfluenceConstraint.java
##########
@@ -0,0 +1,54 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Map;
+
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+
+
+/**
+ * Evaluate the proposed assignment according to the potential partition movements cost based on
+ * the baseline assignment's influence.
+ * This constraint promotes movements for evenness. If best possible doesn't exist, baseline will be
+ * used to restrict movements, so this constraint should give no score in that case.
+ */
+public class BaselineInfluenceConstraint extends AbstractPartitionMovementConstraint {
+ @Override
+ protected double getAssignmentScore(AssignableNode node, AssignableReplica replica,
+ ClusterContext clusterContext) {
+ Map<String, String> bestPossibleAssignment =
+ getStateMap(replica, clusterContext.getBestPossibleAssignment());
+ Map<String, String> baselineAssignment =
+ getStateMap(replica, clusterContext.getBaselineAssignment());
+ if (bestPossibleAssignment.isEmpty()) {
+ return getMinScore();
+ }
+ return calculateAssignmentScore(node.getInstanceName(), replica.getReplicaState(),
+ baselineAssignment);
+ }
+
+ @Override
+ protected double getStateTransitionCostFactor() {
+ return MAX_SCORE / 2;
Review comment:
1. removing getStateTransitionCostFactor() is the same thing as I commented before that we can make this method non-abstract. So no objection on my side.
2. MAX_SCORE / 2 is hardcode. Not good. What if we find that the cost is higher or lower than we thought? Or we want to make it configurable for different clusters? Then we need to fix it anyway.
3. I think the factor design is easier to maintain and cleaner, considering that we are not 100% sure that 0.5 is a good rate for everyone.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586021526
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/AbstractPartitionMovementConstraint.java
##########
@@ -0,0 +1,86 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Collections;
+import java.util.Map;
+
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.ResourceAssignment;
+
+/**
+ * Evaluate the proposed assignment according to the potential partition movements cost.
+ * The cost is evaluated based on the difference between the old assignment and the new assignment.
+ * Any change from the old assignment will increase the partition movements cost, so that the
+ * evaluated score will become lower.
+ */
+abstract class AbstractPartitionMovementConstraint extends SoftConstraint {
+ protected static final double MAX_SCORE = 1f;
+ protected static final double MIN_SCORE = 0f;
+
+ AbstractPartitionMovementConstraint() {
+ super(MAX_SCORE, MIN_SCORE);
+ }
+
+ /**
+ * @return MAX_SCORE if the proposed assignment completely matches the previous assignment.
+ * StateTransitionCostFactor if the proposed assignment's allocation matches the
+ * previous assignment but state does not match.
+ * MIN_SCORE if the proposed assignment completely doesn't match the previous one.
+ */
+ @Override
+ protected abstract double getAssignmentScore(AssignableNode node, AssignableReplica replica,
+ ClusterContext clusterContext);
+
+ /**
+ * @return The scale factor to adjust score when the proposed allocation partially matches the
+ * assignment plan but will require a state transition (with partition movement).
+ */
+ protected abstract double getStateTransitionCostFactor();
+
+ protected Map<String, String> getStateMap(AssignableReplica replica,
+ Map<String, ResourceAssignment> assignment) {
+ String resourceName = replica.getResourceName();
+ String partitionName = replica.getPartitionName();
+ if (assignment == null || !assignment.containsKey(resourceName)) {
+ return Collections.emptyMap();
+ }
+ return assignment.get(resourceName).getReplicaMap(new Partition(partitionName));
+ }
+
+ protected double calculateAssignmentScore(String nodeName, String state,
+ Map<String, String> instanceToStateMap) {
+ if (instanceToStateMap.containsKey(nodeName)) {
+ return state.equals(instanceToStateMap.get(nodeName)) ?
+ MAX_SCORE : // if state matches, no state transition required for the proposed assignment
+ getStateTransitionCostFactor(); // if state does not match, then the proposed assignment
Review comment:
That's a valid point. Ok.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on pull request #1658:
URL: https://github.com/apache/helix/pull/1658#issuecomment-793082144
This PR is ready to be merged, approved by @jiajunwang
Final commit message:
## Add new PartitonMovementConstraint and BaselineInfluenceConstraint for Waged ##
This PR splits PartitionMovementConstraint into separate constraints that control baseline convergence and best possible movements respectively.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585952168
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -73,19 +97,13 @@ protected double getAssignmentScore(AssignableNode node, AssignableReplica repli
if (bestPossibleAssignment.isEmpty()) {
// If bestPossibleAssignment of the replica is empty, indicating this is a new replica.
// Then the baseline is the only reference.
- return calculateAssignmentScore(nodeName, state, baselineAssignment);
+ return calculateAssignmentScore(nodeName, state, baselineAssignment)
Review comment:
Outdated.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585934994
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -19,48 +19,20 @@
* under the License.
*/
-import java.util.Collections;
import java.util.Map;
import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
-import org.apache.helix.model.Partition;
-import org.apache.helix.model.ResourceAssignment;
+
/**
- * Evaluate the proposed assignment according to the potential partition movements cost.
- * The cost is evaluated based on the difference between the old assignment and the new assignment.
- * In detail, we consider the following two previous assignments as the base.
- * - Baseline assignment that is calculated regardless of the node state (online/offline).
- * - Previous Best Possible assignment.
- * Any change to these two assignments will increase the partition movements cost, so that the
- * evaluated score will become lower.
+ * Evaluate the proposed assignment according to the potential partition movements cost based on
+ * the previous best possible assignment.
+ * The previous best possible assignment is the sole reference, and if it's missing, use baseline
+ * assignment instead.
*/
-class PartitionMovementConstraint extends SoftConstraint {
- private static final double MAX_SCORE = 1f;
- private static final double MIN_SCORE = 0f;
- // 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
Review comment:
It has been moved to `AbstractPartitionMovementConstraint`
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang merged pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1658:
URL: https://github.com/apache/helix/pull/1658
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r587823050
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##########
@@ -61,19 +66,33 @@ public static RebalanceAlgorithm getInstance(
new ReplicaActivateConstraint(), new NodeMaxPartitionLimitConstraint(),
new ValidGroupTagConstraint(), new SamePartitionOnInstanceConstraint());
- int evennessPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS, 1);
- int movementPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT, 1);
+ int evennessPreference = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS,
+ ClusterConfig.DEFAULT_GLOBAL_REBALANCE_PREFERENCE
+ .get(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS));
+ int movementPreference = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT,
+ ClusterConfig.DEFAULT_GLOBAL_REBALANCE_PREFERENCE
+ .get(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT));
+
+ boolean forceBaselineConverge = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.FORCE_BASELINE_CONVERGE, 0)
+ > 0;
+ if (forceBaselineConverge) {
+ MODEL.put(BaselineInfluenceConstraint.class.getSimpleName(), FORCE_BASELINE_CONVERGE_WEIGHT);
Review comment:
Makes sense, changing.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585955260
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -71,48 +43,13 @@ protected double getAssignmentScore(AssignableNode node, AssignableReplica repli
String state = replica.getReplicaState();
if (bestPossibleAssignment.isEmpty()) {
- // If bestPossibleAssignment of the replica is empty, indicating this is a new replica.
Review comment:
Yeah, but it is OK just leave it here since it is still valid.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586714071
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -122,13 +122,6 @@ public void testGetRebalancePreferenceDefault() {
ClusterConfig testConfig = new ClusterConfig("testId");
Assert.assertEquals(testConfig.getGlobalRebalancePreference(),
ClusterConfig.DEFAULT_GLOBAL_REBALANCE_PREFERENCE);
-
- Map<ClusterConfig.GlobalRebalancePreferenceKey, Integer> preference = new HashMap<>();
Review comment:
Moving back to ClusterConfig to validate during `set()`. The validation is done there: either both evenness and less_movement are specified or neither of them are specified; the validation is guarded with a `IllegalArgumentException`.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586826432
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -171,6 +164,15 @@ public void testSetRebalancePreferenceInvalidNumber() {
testConfig.setGlobalRebalancePreference(preference);
}
+ @Test(expectedExceptions = IllegalArgumentException.class)
+ public void testSetRebalancePreferenceMissingKey() {
+ Map<ClusterConfig.GlobalRebalancePreferenceKey, Integer> preference = new HashMap<>();
+ preference.put(EVENNESS, 1);
+
+ ClusterConfig testConfig = new ClusterConfig("testId");
+ testConfig.setGlobalRebalancePreference(preference);
Review comment:
In general, I agree with the design. But this adds a more strict requirement on the user settings. Chances are some cluster configured with a weird setup (which works before), will fail. Do we need to be concerned about this?
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -862,14 +865,22 @@ private void setDefaultCapacityMap(ClusterConfigProperty capacityPropertyType,
/**
* Set the global rebalancer's assignment preference.
- * @param preference A map of the GlobalRebalancePreferenceKey and the corresponding weight.
- * The ratio of the configured weights will determine the rebalancer's behavior.
+ * @param preference A map of the GlobalRebalancePreferenceKey and the corresponding weights.
+ * The weights will determine the rebalancer's behavior. Note that
+ * GlobalRebalancePreferenceKey.EVENNESS and
+ * GlobalRebalancePreferenceKey.LESS_MOVEMENT must be both specified or not
+ * specified, or an exception will be thrown.
* If null, the preference item will be removed from the config.
*/
public void setGlobalRebalancePreference(Map<GlobalRebalancePreferenceKey, Integer> preference) {
if (preference == null) {
_record.getMapFields().remove(ClusterConfigProperty.REBALANCE_PREFERENCE.name());
} else {
+ if (preference.containsKey(GlobalRebalancePreferenceKey.EVENNESS) != preference
Review comment:
As I commented in the test file, this is a more strict restriction. There is a backward compatibility concern.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/AbstractPartitionMovementConstraint.java
##########
@@ -0,0 +1,86 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Collections;
+import java.util.Map;
+
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.ResourceAssignment;
+
+/**
+ * Evaluate the proposed assignment according to the potential partition movements cost.
+ * The cost is evaluated based on the difference between the old assignment and the new assignment.
+ * Any change from the old assignment will increase the partition movements cost, so that the
+ * evaluated score will become lower.
+ */
+abstract class AbstractPartitionMovementConstraint extends SoftConstraint {
+ protected static final double MAX_SCORE = 1f;
+ protected static final double MIN_SCORE = 0f;
+
+ private static final double STATE_TRANSITION_COST_FACTOR = 0.5;
+
+ AbstractPartitionMovementConstraint() {
+ super(MAX_SCORE, MIN_SCORE);
+ }
+
+ /**
+ * @return MAX_SCORE if the proposed assignment completely matches the previous assignment.
+ * StateTransitionCostFactor if the proposed assignment's allocation matches the
+ * previous assignment but state does not match.
+ * MIN_SCORE if the proposed assignment completely doesn't match the previous one.
+ */
+ @Override
+ protected abstract double getAssignmentScore(AssignableNode node, AssignableReplica replica,
+ ClusterContext clusterContext);
+
+ protected Map<String, String> getStateMap(AssignableReplica replica,
+ Map<String, ResourceAssignment> assignment) {
+ String resourceName = replica.getResourceName();
+ String partitionName = replica.getPartitionName();
+ if (assignment == null || !assignment.containsKey(resourceName)) {
+ return Collections.emptyMap();
+ }
+ return assignment.get(resourceName).getReplicaMap(new Partition(partitionName));
+ }
+
+ protected double calculateAssignmentScore(String nodeName, String state,
+ Map<String, String> instanceToStateMap) {
+ if (instanceToStateMap.containsKey(nodeName)) {
+ // The score when the proposed allocation partially matches the assignment plan but will
+ // require a state transition.
+ double scoreWithStateTransitionCost =
+ MIN_SCORE + (MAX_SCORE - MIN_SCORE) * STATE_TRANSITION_COST_FACTOR;
Review comment:
nit, purely for style. So if we used the static value here, we shall use it everywhere in this class. I see there is one mixed usage which calls getMaxScore(). Let's do either all static values or getXXXScore() methods.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##########
@@ -61,19 +66,33 @@ public static RebalanceAlgorithm getInstance(
new ReplicaActivateConstraint(), new NodeMaxPartitionLimitConstraint(),
new ValidGroupTagConstraint(), new SamePartitionOnInstanceConstraint());
- int evennessPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS, 1);
- int movementPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT, 1);
+ int evennessPreference = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS,
+ ClusterConfig.DEFAULT_GLOBAL_REBALANCE_PREFERENCE
+ .get(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS));
+ int movementPreference = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT,
+ ClusterConfig.DEFAULT_GLOBAL_REBALANCE_PREFERENCE
+ .get(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT));
+
+ boolean forceBaselineConverge = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.FORCE_BASELINE_CONVERGE, 0)
+ > 0;
+ if (forceBaselineConverge) {
+ MODEL.put(BaselineInfluenceConstraint.class.getSimpleName(), FORCE_BASELINE_CONVERGE_WEIGHT);
Review comment:
I still suggest moving this after the soft constraint weight adjustment, and directly set to softConstraintsWithWeight instead of modifying the MODEL.
1. The 3 preference configurations shall be clearly defined. In theory, FORCE_BASELINE_CONVERGE
is not related to EVENNESS or LESS_MOVEMENT. So we shall not let its static weight being polluted by the EVENNESS preference.
2. MODEL is a static map, but FORCE_BASELINE_CONVERGE can be changed dynamically. You cannot just enable it, but you will need to check and remove it if FORCE_BASELINE_CONVERGE is not setup. This is low efficiency. Better to directly put it into the dynamic map softConstraintsWithWeight. This is computed from scratch every time anyway.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586005518
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/AbstractPartitionMovementConstraint.java
##########
@@ -0,0 +1,86 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Collections;
+import java.util.Map;
+
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.ResourceAssignment;
+
+/**
+ * Evaluate the proposed assignment according to the potential partition movements cost.
+ * The cost is evaluated based on the difference between the old assignment and the new assignment.
+ * Any change from the old assignment will increase the partition movements cost, so that the
+ * evaluated score will become lower.
+ */
+abstract class AbstractPartitionMovementConstraint extends SoftConstraint {
+ protected static final double MAX_SCORE = 1f;
+ protected static final double MIN_SCORE = 0f;
+
+ AbstractPartitionMovementConstraint() {
+ super(MAX_SCORE, MIN_SCORE);
+ }
+
+ /**
+ * @return MAX_SCORE if the proposed assignment completely matches the previous assignment.
+ * StateTransitionCostFactor if the proposed assignment's allocation matches the
+ * previous assignment but state does not match.
+ * MIN_SCORE if the proposed assignment completely doesn't match the previous one.
+ */
+ @Override
+ protected abstract double getAssignmentScore(AssignableNode node, AssignableReplica replica,
+ ClusterContext clusterContext);
+
+ /**
+ * @return The scale factor to adjust score when the proposed allocation partially matches the
+ * assignment plan but will require a state transition (with partition movement).
+ */
+ protected abstract double getStateTransitionCostFactor();
+
+ protected Map<String, String> getStateMap(AssignableReplica replica,
+ Map<String, ResourceAssignment> assignment) {
+ String resourceName = replica.getResourceName();
+ String partitionName = replica.getPartitionName();
+ if (assignment == null || !assignment.containsKey(resourceName)) {
+ return Collections.emptyMap();
+ }
+ return assignment.get(resourceName).getReplicaMap(new Partition(partitionName));
+ }
+
+ protected double calculateAssignmentScore(String nodeName, String state,
+ Map<String, String> instanceToStateMap) {
+ if (instanceToStateMap.containsKey(nodeName)) {
+ return state.equals(instanceToStateMap.get(nodeName)) ?
+ MAX_SCORE : // if state matches, no state transition required for the proposed assignment
+ getStateTransitionCostFactor(); // if state does not match, then the proposed assignment
Review comment:
It is OK, but I don't like that design. First of all, I think we all agree that the logic would be the same.
My concern is that factor design is bug free. By returning a number between 0-1, the eventual score would be sure to be between MIN_SCORE and MAX_SCORE. However, if you directly return the getStateTransitionCost(), then we rely on the coder's discipline.
Actually, even you change the method to be getStateTransitionCost(), I think it shall still be implemented with the above formula,
(MAX_SCORE - MIN_SCORE) * STATE_TRANSITION_COST_FACTOR + MIN_SCORE
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r587819613
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -171,6 +164,15 @@ public void testSetRebalancePreferenceInvalidNumber() {
testConfig.setGlobalRebalancePreference(preference);
}
+ @Test(expectedExceptions = IllegalArgumentException.class)
+ public void testSetRebalancePreferenceMissingKey() {
+ Map<ClusterConfig.GlobalRebalancePreferenceKey, Integer> preference = new HashMap<>();
+ preference.put(EVENNESS, 1);
+
+ ClusterConfig testConfig = new ClusterConfig("testId");
+ testConfig.setGlobalRebalancePreference(preference);
Review comment:
Before, if they set a mapping with only one side defined, it's actually failing silently, in the sense that it never worked - the default weights were used. I think this is a step-up: we are now alerting users of this behavior that was previously silent.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586017951
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -160,7 +161,8 @@
DEFAULT_GLOBAL_REBALANCE_PREFERENCE =
Review comment:
It's still used by `getGlobalRebalancePreference()` in the config class. I think it's fine where it is, because it has the format of a map, and logically belongs to ClusterConfig. Technically its default values should belong in the factory class, but that may be an overkill.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586714071
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -122,13 +122,6 @@ public void testGetRebalancePreferenceDefault() {
ClusterConfig testConfig = new ClusterConfig("testId");
Assert.assertEquals(testConfig.getGlobalRebalancePreference(),
ClusterConfig.DEFAULT_GLOBAL_REBALANCE_PREFERENCE);
-
- Map<ClusterConfig.GlobalRebalancePreferenceKey, Integer> preference = new HashMap<>();
Review comment:
Moving back to ClusterConfig to validate during `set()`.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585933048
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##########
@@ -61,21 +61,34 @@ public static RebalanceAlgorithm getInstance(
new ReplicaActivateConstraint(), new NodeMaxPartitionLimitConstraint(),
new ValidGroupTagConstraint(), new SamePartitionOnInstanceConstraint());
- int evennessPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS, 1);
- int movementPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT, 1);
+ int evennessPreference = 1;
+ int movementPreference = 1;
+ if (preferences.containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS) && preferences
+ .containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS)) {
+ evennessPreference = preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS);
+ movementPreference =
+ preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT);
+ }
+
+ boolean forceBaselineConverge = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.FORCE_BASELINE_CONVERGE, 0)
+ > 0;
+ if (forceBaselineConverge) {
+ MODEL.put(PartitionMovementConstraint.class.getSimpleName(), 0.001f);
Review comment:
For the cluster that motivates this change, an extremely small number like 0.001 is required to achieve better baseline evenness than CrushED. The quality of the baseline is only controlled by this factor.
Note that we cannot set the 0.001 with preferences, since they must be integers. This part is where I find this approach having a hard time creating something elegant, unless we use system properties to overwrite.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586005518
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/AbstractPartitionMovementConstraint.java
##########
@@ -0,0 +1,86 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Collections;
+import java.util.Map;
+
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.ResourceAssignment;
+
+/**
+ * Evaluate the proposed assignment according to the potential partition movements cost.
+ * The cost is evaluated based on the difference between the old assignment and the new assignment.
+ * Any change from the old assignment will increase the partition movements cost, so that the
+ * evaluated score will become lower.
+ */
+abstract class AbstractPartitionMovementConstraint extends SoftConstraint {
+ protected static final double MAX_SCORE = 1f;
+ protected static final double MIN_SCORE = 0f;
+
+ AbstractPartitionMovementConstraint() {
+ super(MAX_SCORE, MIN_SCORE);
+ }
+
+ /**
+ * @return MAX_SCORE if the proposed assignment completely matches the previous assignment.
+ * StateTransitionCostFactor if the proposed assignment's allocation matches the
+ * previous assignment but state does not match.
+ * MIN_SCORE if the proposed assignment completely doesn't match the previous one.
+ */
+ @Override
+ protected abstract double getAssignmentScore(AssignableNode node, AssignableReplica replica,
+ ClusterContext clusterContext);
+
+ /**
+ * @return The scale factor to adjust score when the proposed allocation partially matches the
+ * assignment plan but will require a state transition (with partition movement).
+ */
+ protected abstract double getStateTransitionCostFactor();
+
+ protected Map<String, String> getStateMap(AssignableReplica replica,
+ Map<String, ResourceAssignment> assignment) {
+ String resourceName = replica.getResourceName();
+ String partitionName = replica.getPartitionName();
+ if (assignment == null || !assignment.containsKey(resourceName)) {
+ return Collections.emptyMap();
+ }
+ return assignment.get(resourceName).getReplicaMap(new Partition(partitionName));
+ }
+
+ protected double calculateAssignmentScore(String nodeName, String state,
+ Map<String, String> instanceToStateMap) {
+ if (instanceToStateMap.containsKey(nodeName)) {
+ return state.equals(instanceToStateMap.get(nodeName)) ?
+ MAX_SCORE : // if state matches, no state transition required for the proposed assignment
+ getStateTransitionCostFactor(); // if state does not match, then the proposed assignment
Review comment:
It is OK, but I don't like that design. First of all, I think we all agree that the logic would be the same.
My concern is that factor design is bug free. By returning a number between 0-1, the eventual score would be sure to be between MIN_SCORE and MAX_SCORE. However, if you directly return the getStateTransitionCost(), then we rely on the coder's discipline.
Actually, even you change the method to be getStateTransitionCost(), I think it shall still be implemented with the above formula,
(MAX_SCORE - MIN_SCORE) * getStateTransitionCostFactor() + MIN_SCORE
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586000058
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -122,13 +122,6 @@ public void testGetRebalancePreferenceDefault() {
ClusterConfig testConfig = new ClusterConfig("testId");
Assert.assertEquals(testConfig.getGlobalRebalancePreference(),
ClusterConfig.DEFAULT_GLOBAL_REBALANCE_PREFERENCE);
-
- Map<ClusterConfig.GlobalRebalancePreferenceKey, Integer> preference = new HashMap<>();
Review comment:
Either way works. But we need to make it clean. The validation should be done in one place. So ConstraintBasedAlgorithmFactory might be a better option given the logic becomes more complex. But I don't have a strong preference.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -19,48 +19,20 @@
* under the License.
*/
-import java.util.Collections;
import java.util.Map;
import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
-import org.apache.helix.model.Partition;
-import org.apache.helix.model.ResourceAssignment;
+
/**
- * Evaluate the proposed assignment according to the potential partition movements cost.
- * The cost is evaluated based on the difference between the old assignment and the new assignment.
- * In detail, we consider the following two previous assignments as the base.
- * - Baseline assignment that is calculated regardless of the node state (online/offline).
- * - Previous Best Possible assignment.
- * Any change to these two assignments will increase the partition movements cost, so that the
- * evaluated score will become lower.
+ * Evaluate the proposed assignment according to the potential partition movements cost based on
+ * the previous best possible assignment.
+ * The previous best possible assignment is the sole reference, and if it's missing, use baseline
+ * assignment instead.
*/
-class PartitionMovementConstraint extends SoftConstraint {
- private static final double MAX_SCORE = 1f;
- private static final double MIN_SCORE = 0f;
- // 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
Review comment:
I mean @Override
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -160,7 +161,8 @@
DEFAULT_GLOBAL_REBALANCE_PREFERENCE =
Review comment:
Yeah, with your revised version, it looks good now. Before, since you hardcoded the value there, it is not really using the default numbers.
There is one more thing I suggest to change. It is not used anymore in this file. So we shall move it to ConstraintBasedAlgorithmFactory.java. Since the default preference is no longer part of the config class logic anymore.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585936135
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -71,48 +43,13 @@ protected double getAssignmentScore(AssignableNode node, AssignableReplica repli
String state = replica.getReplicaState();
if (bestPossibleAssignment.isEmpty()) {
- // If bestPossibleAssignment of the replica is empty, indicating this is a new replica.
Review comment:
I have already added that to the javadoc of this constraint. I can reiterate it, sure.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585929074
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/BaselineInfluenceConstraint.java
##########
@@ -0,0 +1,54 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Map;
+
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+
+
+/**
+ * Evaluate the proposed assignment according to the potential partition movements cost based on
+ * the baseline assignment's influence.
+ * This constraint promotes movements for evenness. If best possible doesn't exist, baseline will be
+ * used to restrict movements, so this constraint should give no score in that case.
+ */
+public class BaselineInfluenceConstraint extends AbstractPartitionMovementConstraint {
+ @Override
+ protected double getAssignmentScore(AssignableNode node, AssignableReplica replica,
+ ClusterContext clusterContext) {
+ Map<String, String> bestPossibleAssignment =
+ getStateMap(replica, clusterContext.getBestPossibleAssignment());
+ Map<String, String> baselineAssignment =
+ getStateMap(replica, clusterContext.getBaselineAssignment());
+ if (bestPossibleAssignment.isEmpty()) {
+ return getMinScore();
+ }
+ return calculateAssignmentScore(node.getInstanceName(), replica.getReplicaState(),
+ baselineAssignment);
+ }
+
+ @Override
+ protected double getStateTransitionCostFactor() {
+ return MAX_SCORE / 2;
Review comment:
Mentioned this in an earlier comment, I will rename the factor to a score, since that was the old behavior.
I honestly think this way is fine, because it's clearer. The abstract function's methods act as the default implementation: it returns MAX_SCORE if totally match, MAX_SCORE/2 if requires state transtion, MIN_SCORE for anything else. If the extended function wants to be different, it can just override.
Maybe I can even remove getStateTransitionCostFactor() and just put the default value in calculateAssignmentScore(). What do you think?
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586645705
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/ConstraintBasedAlgorithmFactory.java
##########
@@ -61,21 +61,34 @@ public static RebalanceAlgorithm getInstance(
new ReplicaActivateConstraint(), new NodeMaxPartitionLimitConstraint(),
new ValidGroupTagConstraint(), new SamePartitionOnInstanceConstraint());
- int evennessPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS, 1);
- int movementPreference =
- preferences.getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT, 1);
+ int evennessPreference = 1;
+ int movementPreference = 1;
+ if (preferences.containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS) && preferences
+ .containsKey(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS)) {
+ evennessPreference = preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.EVENNESS);
+ movementPreference =
+ preferences.get(ClusterConfig.GlobalRebalancePreferenceKey.LESS_MOVEMENT);
+ }
+
+ boolean forceBaselineConverge = preferences
+ .getOrDefault(ClusterConfig.GlobalRebalancePreferenceKey.FORCE_BASELINE_CONVERGE, 0)
+ > 0;
+ if (forceBaselineConverge) {
+ MODEL.put(PartitionMovementConstraint.class.getSimpleName(), 0.001f);
Review comment:
Conclusion: for this PR, I'm removing this line. Another solution will be introduced later if necessary.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r585925068
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/AbstractPartitionMovementConstraint.java
##########
@@ -0,0 +1,86 @@
+package org.apache.helix.controller.rebalancer.waged.constraints;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Collections;
+import java.util.Map;
+
+import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
+import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
+import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.ResourceAssignment;
+
+/**
+ * Evaluate the proposed assignment according to the potential partition movements cost.
+ * The cost is evaluated based on the difference between the old assignment and the new assignment.
+ * Any change from the old assignment will increase the partition movements cost, so that the
+ * evaluated score will become lower.
+ */
+abstract class AbstractPartitionMovementConstraint extends SoftConstraint {
+ protected static final double MAX_SCORE = 1f;
+ protected static final double MIN_SCORE = 0f;
+
+ AbstractPartitionMovementConstraint() {
+ super(MAX_SCORE, MIN_SCORE);
+ }
+
+ /**
+ * @return MAX_SCORE if the proposed assignment completely matches the previous assignment.
+ * StateTransitionCostFactor if the proposed assignment's allocation matches the
+ * previous assignment but state does not match.
+ * MIN_SCORE if the proposed assignment completely doesn't match the previous one.
+ */
+ @Override
+ protected abstract double getAssignmentScore(AssignableNode node, AssignableReplica replica,
+ ClusterContext clusterContext);
+
+ /**
+ * @return The scale factor to adjust score when the proposed allocation partially matches the
+ * assignment plan but will require a state transition (with partition movement).
+ */
+ protected abstract double getStateTransitionCostFactor();
+
+ protected Map<String, String> getStateMap(AssignableReplica replica,
+ Map<String, ResourceAssignment> assignment) {
+ String resourceName = replica.getResourceName();
+ String partitionName = replica.getPartitionName();
+ if (assignment == null || !assignment.containsKey(resourceName)) {
+ return Collections.emptyMap();
+ }
+ return assignment.get(resourceName).getReplicaMap(new Partition(partitionName));
+ }
+
+ protected double calculateAssignmentScore(String nodeName, String state,
+ Map<String, String> instanceToStateMap) {
+ if (instanceToStateMap.containsKey(nodeName)) {
+ return state.equals(instanceToStateMap.get(nodeName)) ?
+ MAX_SCORE : // if state matches, no state transition required for the proposed assignment
+ getStateTransitionCostFactor(); // if state does not match, then the proposed assignment
Review comment:
I'm reusing the old name, but it's really just the score (0.5), not a scaling factor. I'm renaming the method to clearly indicate that it's returning a score, not a scaling factor.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1658: New PartitionMovementConstraint and BaselineInfluenceConstraint for Waged
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1658:
URL: https://github.com/apache/helix/pull/1658#discussion_r586015496
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/constraints/PartitionMovementConstraint.java
##########
@@ -19,48 +19,20 @@
* under the License.
*/
-import java.util.Collections;
import java.util.Map;
import org.apache.helix.controller.rebalancer.waged.model.AssignableNode;
import org.apache.helix.controller.rebalancer.waged.model.AssignableReplica;
import org.apache.helix.controller.rebalancer.waged.model.ClusterContext;
-import org.apache.helix.model.Partition;
-import org.apache.helix.model.ResourceAssignment;
+
/**
- * Evaluate the proposed assignment according to the potential partition movements cost.
- * The cost is evaluated based on the difference between the old assignment and the new assignment.
- * In detail, we consider the following two previous assignments as the base.
- * - Baseline assignment that is calculated regardless of the node state (online/offline).
- * - Previous Best Possible assignment.
- * Any change to these two assignments will increase the partition movements cost, so that the
- * evaluated score will become lower.
+ * Evaluate the proposed assignment according to the potential partition movements cost based on
+ * the previous best possible assignment.
+ * The previous best possible assignment is the sole reference, and if it's missing, use baseline
+ * assignment instead.
*/
-class PartitionMovementConstraint extends SoftConstraint {
- private static final double MAX_SCORE = 1f;
- private static final double MIN_SCORE = 0f;
- // 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
Review comment:
Ah, fixed.
----------------------------------------------------------------
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