dasahcc commented on a change in pull request #1098:
URL: https://github.com/apache/helix/pull/1098#discussion_r441755853
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/AbstractRebalancer.java
##########
@@ -385,10 +389,19 @@ public static int getStateCount(String state,
StateModelDefinition stateModelDef
// If the desired state is the top state, but the instance cannot be
transited to the
// top state in one hop, try to keep the top state on current host
or a host with a closer
// state.
- for (String currentStatePrioritizedInstance : sortedPreferenceList) {
- if (!assigned.contains(currentStatePrioritizedInstance) &&
liveAndEnabled
- .contains(currentStatePrioritizedInstance)) {
+ while (currentStatePrioritizedInstanceIter.hasNext()) {
+ // Note that it is safe to check the prioritized instance items
only once here.
+ // Since the only possible condition when we don't use an instance
in this list is that
+ // it has been assigned with some state. And this is not
revertable in this method. So
+ // checking it one more time later will only waste time.
+ String currentStatePrioritizedInstance =
currentStatePrioritizedInstanceIter.next();
+ if (!assigned.contains(currentStatePrioritizedInstance)) {
Review comment:
Is there any reasons to remove the liveAndEnabled check?
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/AbstractRebalancer.java
##########
@@ -385,10 +389,19 @@ public static int getStateCount(String state,
StateModelDefinition stateModelDef
// If the desired state is the top state, but the instance cannot be
transited to the
// top state in one hop, try to keep the top state on current host
or a host with a closer
// state.
- for (String currentStatePrioritizedInstance : sortedPreferenceList) {
- if (!assigned.contains(currentStatePrioritizedInstance) &&
liveAndEnabled
- .contains(currentStatePrioritizedInstance)) {
+ while (currentStatePrioritizedInstanceIter.hasNext()) {
+ // Note that it is safe to check the prioritized instance items
only once here.
+ // Since the only possible condition when we don't use an instance
in this list is that
+ // it has been assigned with some state. And this is not
revertable in this method. So
+ // checking it one more time later will only waste time.
+ String currentStatePrioritizedInstance =
currentStatePrioritizedInstanceIter.next();
+ if (!assigned.contains(currentStatePrioritizedInstance)) {
proposedInstance = currentStatePrioritizedInstance;
+ // If we find a different instance for the partition placement,
then we need to check
+ // the same instance again or it will not be assigned with any
partitions.
+ if (!proposedInstance.equals(instance)) {
+ i--;
Review comment:
Using index will be hard for code reader to understand it. Suggest to
using a queue to replace the for loop.
So it could be: while (!queue.isEmpty()) {
xxxxx
if (!proposedInstance.equals(instance)) {
queue.offer(instance); // add back
}
}
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/AbstractRebalancer.java
##########
@@ -385,10 +389,19 @@ public static int getStateCount(String state,
StateModelDefinition stateModelDef
// If the desired state is the top state, but the instance cannot be
transited to the
// top state in one hop, try to keep the top state on current host
or a host with a closer
// state.
- for (String currentStatePrioritizedInstance : sortedPreferenceList) {
- if (!assigned.contains(currentStatePrioritizedInstance) &&
liveAndEnabled
- .contains(currentStatePrioritizedInstance)) {
+ while (currentStatePrioritizedInstanceIter.hasNext()) {
+ // Note that it is safe to check the prioritized instance items
only once here.
+ // Since the only possible condition when we don't use an instance
in this list is that
+ // it has been assigned with some state. And this is not
revertable in this method. So
+ // checking it one more time later will only waste time.
+ String currentStatePrioritizedInstance =
currentStatePrioritizedInstanceIter.next();
Review comment:
Maybe I am wrong. But could that be a scenario of:
preference list = 1, 2, 3
current state = 4, 5, 6
Then we keep assign ONLINE to 4,5,6. There will not be any movement. For
DelayedAutoRebalance we may dont have it. But for default AutoRebalancer Or
SemiAuto, we may use this for AbstractRebalancer then there will not be any
movement.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
