junkaixue commented on code in PR #2546:
URL: https://github.com/apache/helix/pull/2546#discussion_r1242678664
##########
helix-core/src/main/java/org/apache/helix/controller/dataproviders/InstanceCapacityDataProvider.java:
##########
@@ -42,18 +42,10 @@ public interface InstanceCapacityDataProvider {
/**
* Check if partition can be placed on the instance.
*
- * @param instanceName - instance name
+ * @param instanceName - instance name
* @param partitionCapacity - Partition capacity expresed in capacity map.
* @return boolean - True if the partition can be placed, False otherwise
*/
public boolean isInstanceCapacityAvailable(String instanceName, Map<String,
Integer> partitionCapacity);
- /**
- * Reduce the available capacity by specified Partition Capacity Map.
- *
- * @param instanceName - instance name
- * @param partitionCapacity - Partition capacity expresed in capacity map.
- * @returns boolean - True if successfully updated partition capacity, false
otherwise.
- */
- public boolean reduceAvailableInstanceCapacity(String instanceName,
Map<String, Integer> partitionCapacity);
Review Comment:
Question: this is the one just added before, right? If it already exists in
last release, we cannot delete it.
##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/DelayedAutoRebalancer.java:
##########
@@ -408,4 +420,129 @@ private int getNumExtraReplicas(ClusterConfig
clusterConfig) {
}
return numExtraReplicas;
}
+
+
+ private Map<String, String>
computeBestPossibleStateForPartition(ResourceControllerDataProvider cache,
Review Comment:
This function has a lot of redundant code with existing logic. I would
suggest to simplify it.
##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/DelayedAutoRebalancer.java:
##########
@@ -408,4 +420,129 @@ private int getNumExtraReplicas(ClusterConfig
clusterConfig) {
}
return numExtraReplicas;
}
+
+
+ private Map<String, String>
computeBestPossibleStateForPartition(ResourceControllerDataProvider cache,
+ StateModelDefinition stateModelDef, List<String> preferenceList,
+ CurrentStateOutput currentStateOutput, Set<String>
disabledInstancesForPartition,
+ IdealState idealState, ClusterConfig clusterConfig, Partition partition,
+ MonitoredAbnormalResolver monitoredResolver) {
+
+ Optional<Map<String, String>> optionalOverwrittenStates =
+ computeStatesOverwriteForPartition(stateModelDef, preferenceList,
currentStateOutput,
+ idealState, partition, monitoredResolver);
+ if (optionalOverwrittenStates.isPresent()) {
+ return optionalOverwrittenStates.get();
+ }
+
+ Map<String, String> currentStateMap = new HashMap<>(
+ currentStateOutput.getCurrentStateMap(idealState.getResourceName(),
partition));
+ // Instances not in preference list but still have active replica, retain
to avoid zero replica during movement
+ List<String> currentInstances = new ArrayList<>(currentStateMap.keySet());
+ Collections.sort(currentInstances);
+ Map<String, String> pendingStates =
+ new
HashMap<>(currentStateOutput.getPendingStateMap(idealState.getResourceName(),
partition));
+ for (String instance : pendingStates.keySet()) {
+ if (!currentStateMap.containsKey(instance)) {
+ currentStateMap.put(instance, stateModelDef.getInitialState());
+ currentInstances.add(instance);
+ }
+ }
+
+ Set<String> instancesToDrop = new HashSet<>();
+ Iterator<String> it = currentInstances.iterator();
+ while (it.hasNext()) {
+ String instance = it.next();
+ String state = currentStateMap.get(instance);
+ if (state == null) {
+ it.remove();
+ instancesToDrop.add(instance); // These instances should be set to
DROPPED after we get bestPossibleStateMap;
+ }
+ }
+
+ // Sort the instancesToMove by their current partition state.
+ // Reason: because the states are assigned to instances in the order
appeared in preferenceList, if we have
+ // [node1:Slave, node2:Master], we want to keep it that way, instead of
assigning Master to node1.
+
+ if (preferenceList == null) {
+ preferenceList = Collections.emptyList();
+ }
+
+ boolean isPreferenceListEmpty = preferenceList.isEmpty();
+
+ int numExtraReplicas = getNumExtraReplicas(clusterConfig);
+
+ // TODO : Keep the behavior consistent with existing state count, change
back to read from idealstate
+ // replicas
+ int numReplicas = preferenceList.size();
+ List<String> instanceToAdd = new ArrayList<>(preferenceList);
+ instanceToAdd.removeAll(currentInstances);
+
+ List<String> combinedPreferenceList = new ArrayList<>();
+
+ if (currentInstances.size() <= numReplicas
+ && numReplicas + numExtraReplicas - currentInstances.size() > 0) {
+ int subListSize = numReplicas + numExtraReplicas -
currentInstances.size();
+ combinedPreferenceList.addAll(instanceToAdd
+ .subList(0, Math.min(subListSize, instanceToAdd.size())));
+ }
+
+ // Make all initial state instance not in preference list to be dropped.
+ Map<String, String> currentMapWithPreferenceList = new
HashMap<>(currentStateMap);
+ currentMapWithPreferenceList.keySet().retainAll(preferenceList);
+
+ combinedPreferenceList.addAll(currentInstances);
+ combinedPreferenceList.sort(new
PreferenceListNodeComparator(currentStateMap, stateModelDef, preferenceList));
+
+ // if preference list is not empty, and we do have new intanceToAdd, we
should check if it has capacity to hold the partition.
+ // if (!isPreferenceListEmpty && combinedPreferenceList.size() >
numReplicas && instanceToAdd.size() > 0) {
Review Comment:
Remove this if it is not required.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
