You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@helix.apache.org by ji...@apache.org on 2020/06/18 19:57:06 UTC
[helix] branch revert-1098-master created (now 3136de2)
This is an automated email from the ASF dual-hosted git repository.
jiajunwang pushed a change to branch revert-1098-master
in repository https://gitbox.apache.org/repos/asf/helix.git.
at 3136de2 Revert "Fix the issue that the instance may not be assigned a replica as expected. (#1098)"
This branch includes the following new commits:
new 3136de2 Revert "Fix the issue that the instance may not be assigned a replica as expected. (#1098)"
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[helix] 01/01: Revert "Fix the issue that the instance may not be
assigned a replica as expected. (#1098)"
Posted by ji...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jiajunwang pushed a commit to branch revert-1098-master
in repository https://gitbox.apache.org/repos/asf/helix.git
commit 3136de21211fcd555be710cfb73e7a9c9d9e7815
Author: Jiajun Wang <18...@users.noreply.github.com>
AuthorDate: Thu Jun 18 12:56:58 2020 -0700
Revert "Fix the issue that the instance may not be assigned a replica as expected. (#1098)"
This reverts commit 35857a8cc025a1efb49943b6ab31dc12622bf986.
---
.../controller/rebalancer/AbstractRebalancer.java | 157 +++++----------------
.../helix/model/BuiltInStateModelDefinitions.java | 1 -
.../helix/model/OnlineOfflineWithBootstrapSMD.java | 74 ----------
.../rebalancer/TestAbstractRebalancer.java | 2 +-
...bstractRebalancer.ComputeBestPossibleState.json | 24 ----
5 files changed, 39 insertions(+), 219 deletions(-)
diff --git a/helix-core/src/main/java/org/apache/helix/controller/rebalancer/AbstractRebalancer.java b/helix-core/src/main/java/org/apache/helix/controller/rebalancer/AbstractRebalancer.java
index 10d9976..f118bcb 100644
--- a/helix-core/src/main/java/org/apache/helix/controller/rebalancer/AbstractRebalancer.java
+++ b/helix-core/src/main/java/org/apache/helix/controller/rebalancer/AbstractRebalancer.java
@@ -24,15 +24,11 @@ import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
-import java.util.Iterator;
import java.util.LinkedHashMap;
-import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
-import java.util.Queue;
import java.util.Set;
-import java.util.stream.Collectors;
import org.apache.helix.HelixDefinedState;
import org.apache.helix.HelixException;
@@ -356,137 +352,60 @@ public abstract class AbstractRebalancer<T extends BaseControllerDataProvider> i
}
// (3) Assign normal states to instances.
- assignStatesToInstances(preferenceList, stateModelDef, currentStateMap, liveInstances,
- disabledInstancesForPartition, bestPossibleStateMap);
-
- return bestPossibleStateMap;
- }
-
- /**
- * Assign the states to the instances listed in the preference list according to inputs.
- * Note that when we choose the top-state (e.g. MASTER) replica for a partition, we prefer to
- * choose it from these replicas which are already in the secondary states (e.g, SLAVE) instead
- * of in lower-state. This is because a replica in secondary state will take shorter time to
- * transition to the top-state, which could minimize the impact to the application's availability.
- * To achieve that, we sort the preferenceList based on CurrentState, by treating top-state and
- * second-states with same priority and rely on the fact that Collections.sort() is stable.
- */
- private void assignStatesToInstances(final List<String> preferenceList,
- final StateModelDefinition stateModelDef, final Map<String, String> currentStateMap,
- final Set<String> liveInstances, final Set<String> disabledInstancesForPartition,
- Map<String, String> bestPossibleStateMap) {
- // Record the assigned instances to avoid double calculating or conflict assignment.
- Set<String> assignedInstances = new HashSet<>();
-
- Set<String> liveAndEnabled =
- liveInstances.stream().filter(instance -> !disabledInstancesForPartition.contains(instance))
- .collect(Collectors.toSet());
-
- Queue<String> preferredActiveInstanceQueue = new LinkedList<>(preferenceList);
- preferredActiveInstanceQueue.retainAll(liveAndEnabled);
- int totalCandidateCount = preferredActiveInstanceQueue.size();
-
- // Sort the preferred instances based on replicas' state priority in the current state.
- // Note that if one instance exists in the current states but not in the preference list, then
- // it won't show in the prioritized list.
- List<String> currentStatePrioritizedList = new ArrayList<>(preferredActiveInstanceQueue);
- currentStatePrioritizedList.sort(new StatePriorityComparator(currentStateMap, stateModelDef));
- Iterator<String> currentStatePrioritizedInstanceIter = currentStatePrioritizedList.iterator();
-
- // Assign the states to the instances that appear in the preference list.
- for (String state : stateModelDef.getStatesPriorityList()) {
+ // When we choose the top-state (e.g. MASTER) replica for a partition, we prefer to choose it from
+ // these replicas which are already in the secondary states (e.g, SLAVE) instead of in lower-state.
+ // This is because a replica in secondary state will take shorter time to transition to the top-state,
+ // which could minimize the impact to the application's availability.
+ // To achieve that, we sort the preferenceList based on CurrentState, by treating top-state and second-states with
+ // same priority and rely on the fact that Collections.sort() is stable.
+ List<String> statesPriorityList = stateModelDef.getStatesPriorityList();
+ Set<String> assigned = new HashSet<>();
+ Set<String> liveAndEnabled = new HashSet<>(liveInstances);
+ liveAndEnabled.removeAll(disabledInstancesForPartition);
+
+ // Sort the instances based on replicas' state priority in the current state
+ List<String> sortedPreferenceList = new ArrayList<>(preferenceList);
+ sortedPreferenceList.sort(new StatePriorityComparator(currentStateMap, stateModelDef));
+
+ // Assign the state to the instances that appear in the preference list.
+ for (String state : statesPriorityList) {
int stateCount =
getStateCount(state, stateModelDef, liveAndEnabled.size(), preferenceList.size());
- while (!preferredActiveInstanceQueue.isEmpty()) {
+ for (String instance : preferenceList) {
if (stateCount <= 0) {
break; // continue assigning for the next state
}
- String peekInstance = preferredActiveInstanceQueue.peek();
- if (assignedInstances.contains(peekInstance)) {
- preferredActiveInstanceQueue.poll();
+ if (assigned.contains(instance) || !liveAndEnabled.contains(instance)) {
continue; // continue checking for the next available instance
}
- String proposedInstance = adjustInstanceIfNecessary(state, peekInstance,
- currentStateMap.getOrDefault(peekInstance, stateModelDef.getInitialState()),
- stateModelDef, assignedInstances, totalCandidateCount - assignedInstances.size(),
- stateCount, currentStatePrioritizedInstanceIter);
-
- if (proposedInstance.equals(peekInstance)) {
- // If the peeked instance is the final decision, then poll it from the queue.
- preferredActiveInstanceQueue.poll();
+ String proposedInstance = instance;
+ // Additional check and alternate the assignment for reducing top state handoff.
+ if (state.equals(stateModelDef.getTopState()) && !stateModelDef.getSecondTopStates()
+ .contains(currentStateMap.getOrDefault(instance, stateModelDef.getInitialState()))) {
+ // 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)) {
+ proposedInstance = currentStatePrioritizedInstance;
+ break;
+ }
+ }
+ // Note that if all the current top state instances are not assignable, then we fallback
+ // to the default logic that assigning the state according to preference list order.
}
- // else, if we found 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.
-
- // Assign the desired state to the proposed instance if not on ERROR state.
+ // Assign the desired state to the proposed instance
if (HelixDefinedState.ERROR.toString().equals(currentStateMap.get(proposedInstance))) {
bestPossibleStateMap.put(proposedInstance, HelixDefinedState.ERROR.toString());
} else {
bestPossibleStateMap.put(proposedInstance, state);
stateCount--;
}
- // Note that in either case, the proposed instance is considered to be assigned with a state
- // by now.
- if (!assignedInstances.add(proposedInstance)) {
- throw new AssertionError(String
- .format("The proposed instance %s has been already assigned before.",
- proposedInstance));
- }
+ assigned.add(proposedInstance);
}
}
- }
-
- /**
- * If the proposed instance may cause unnecessary state transition (according to the current
- * state), check and return a alternative instance to avoid.
- *
- * @param requestedState The requested state.
- * @param proposedInstance The current proposed instance to host the replica
- * with the specified state.
- * @param currentState The current state of the proposed Instance, or init
- * state if the proposed instance does not have an
- * assignment.
- * @param stateModelDef
- * @param assignedInstances
- * @param remainCandidateCount The count of the remaining unassigned instances
- * @param remainRequestCount The count of the remaining replicas that need to be
- * assigned with the given state.
- * @param currentStatePrioritizedInstanceIter The iterator of the prioritized instance list which
- * can be used to find a better alternative instance.
- * @return The alternative instance, or the original proposed instance if adjustment is not
- * necessary.
- */
- private String adjustInstanceIfNecessary(String requestedState, String proposedInstance,
- String currentState, StateModelDefinition stateModelDef, Set<String> assignedInstances,
- int remainCandidateCount, int remainRequestCount,
- Iterator<String> currentStatePrioritizedInstanceIter) {
- String adjustedInstance = proposedInstance;
- // Check and alternate the assignment for reducing top state handoff.
- // 1. If the requested state is not the top state, then it does not worth it to adjust.
- // 2. If all remaining candidates need to be assigned with the the state, then there is no need
- // to adjust.
- // 3. If the proposed instance already has the top state or a secondary state, then adjustment
- // is not necessary.
- if (remainRequestCount < remainCandidateCount && requestedState
- .equals(stateModelDef.getTopState()) && !requestedState.equals(currentState)
- && !stateModelDef.getSecondTopStates().contains(currentState)) {
- // 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.
- 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 a state. And this is not revertable.
- String currentStatePrioritizedInstance = currentStatePrioritizedInstanceIter.next();
- if (!assignedInstances.contains(currentStatePrioritizedInstance)) {
- adjustedInstance = currentStatePrioritizedInstance;
- break;
- }
- }
- // Note that if all the current top state instances are not assignable, then we fallback
- // to the default logic that assigning the state according to preference list order.
- }
- return adjustedInstance;
+ return bestPossibleStateMap;
}
/**
diff --git a/helix-core/src/main/java/org/apache/helix/model/BuiltInStateModelDefinitions.java b/helix-core/src/main/java/org/apache/helix/model/BuiltInStateModelDefinitions.java
index f42c048..e060c67 100644
--- a/helix-core/src/main/java/org/apache/helix/model/BuiltInStateModelDefinitions.java
+++ b/helix-core/src/main/java/org/apache/helix/model/BuiltInStateModelDefinitions.java
@@ -27,7 +27,6 @@ public enum BuiltInStateModelDefinitions {
LeaderStandby(new LeaderStandbySMD()),
StorageSchemata(new StorageSchemataSMD()),
OnlineOffline(new OnlineOfflineSMD()),
- OnlineOfflineWithBootstrap(OnlineOfflineWithBootstrapSMD.build()),
ScheduledTask(new ScheduledTaskSMD()),
Task(new TaskSMD());
diff --git a/helix-core/src/main/java/org/apache/helix/model/OnlineOfflineWithBootstrapSMD.java b/helix-core/src/main/java/org/apache/helix/model/OnlineOfflineWithBootstrapSMD.java
deleted file mode 100644
index 97a2f45..0000000
--- a/helix-core/src/main/java/org/apache/helix/model/OnlineOfflineWithBootstrapSMD.java
+++ /dev/null
@@ -1,74 +0,0 @@
-package org.apache.helix.model;
-
-/*
- * 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 org.apache.helix.HelixDefinedState;
-import org.apache.helix.zookeeper.datamodel.ZNRecord;
-
-/**
- * Helix built-in state model definition based on Online-Offline but with the additional bootstrap
- * state.
- */
-public final class OnlineOfflineWithBootstrapSMD extends StateModelDefinition {
- public static final String name = "OnlineOfflineWithBootstrap";
-
- /**
- * Instantiate from a pre-populated record
- *
- * @param record ZNRecord representing a state model definition
- */
- private OnlineOfflineWithBootstrapSMD(ZNRecord record) {
- super(record);
- }
-
- public enum States {
- ONLINE, BOOTSTRAP, OFFLINE
- }
-
- /**
- * Build OnlineOfflineWithBootstrap state model definition
- *
- * @return
- */
- public static OnlineOfflineWithBootstrapSMD build() {
- Builder builder = new Builder(name);
- // init state
- builder.initialState(States.OFFLINE.name());
-
- // add states
- builder.addState(States.ONLINE.name(), 0);
- builder.addState(States.BOOTSTRAP.name(), 1);
- builder.addState(States.OFFLINE.name(), 2);
- for (HelixDefinedState state : HelixDefinedState.values()) {
- builder.addState(state.name());
- }
-
- // add transitions
- builder.addTransition(States.ONLINE.name(), States.OFFLINE.name(), 0);
- builder.addTransition(States.OFFLINE.name(), States.BOOTSTRAP.name(), 1);
- builder.addTransition(States.BOOTSTRAP.name(), States.ONLINE.name(), 2);
- builder.addTransition(States.OFFLINE.name(), HelixDefinedState.DROPPED.name());
-
- // bounds
- builder.dynamicUpperBound(States.ONLINE.name(), "R");
-
- return new OnlineOfflineWithBootstrapSMD(builder.build().getRecord());
- }
-}
diff --git a/helix-core/src/test/java/org/apache/helix/controller/rebalancer/TestAbstractRebalancer.java b/helix-core/src/test/java/org/apache/helix/controller/rebalancer/TestAbstractRebalancer.java
index 513fd2c..6ecaa30 100644
--- a/helix-core/src/test/java/org/apache/helix/controller/rebalancer/TestAbstractRebalancer.java
+++ b/helix-core/src/test/java/org/apache/helix/controller/rebalancer/TestAbstractRebalancer.java
@@ -56,7 +56,7 @@ public class TestAbstractRebalancer {
new IdealState("test"), new ClusterConfig("TestCluster"), partition,
MonitoredAbnormalResolver.DUMMY_STATE_RESOLVER);
- Assert.assertTrue(bestPossibleMap.equals(expectedBestPossibleMap));
+ Assert.assertEquals(bestPossibleMap, expectedBestPossibleMap);
}
@DataProvider(name = "TestComputeBestPossibleStateInput")
diff --git a/helix-core/src/test/resources/TestAbstractRebalancer.ComputeBestPossibleState.json b/helix-core/src/test/resources/TestAbstractRebalancer.ComputeBestPossibleState.json
index 3659942..8aac5c8 100644
--- a/helix-core/src/test/resources/TestAbstractRebalancer.ComputeBestPossibleState.json
+++ b/helix-core/src/test/resources/TestAbstractRebalancer.ComputeBestPossibleState.json
@@ -198,29 +198,5 @@
"node_3": "DROPPED",
"node_4": "SLAVE"
}
- }, {
- "comment": "For a multiple top state state model with complicated middle states, the rebalancer correctly handles the top states and assigns all instances with the expected state.",
- "stateModel": "OnlineOfflineWithBootstrap",
- "liveInstances": [
- "node_1",
- "node_2",
- "node_3"
- ],
- "preferenceList": [
- "node_3",
- "node_2",
- "node_1"
- ],
- "currentStateMap": {
- "node_1": "OFFLINE",
- "node_2": "ONLINE",
- "node_3": "ONLINE"
- },
- "disabledInstancesForPartition": [],
- "expectedBestPossibleStateMap": {
- "node_1": "ONLINE",
- "node_2": "ONLINE",
- "node_3": "ONLINE"
- }
}
]