jiajunwang commented on a change in pull request #519: Refine the rebalance
scope calculating logic in the WAGED rebalancer.
URL: https://github.com/apache/helix/pull/519#discussion_r337859496
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModelProvider.java
##########
@@ -97,27 +97,32 @@ public static ClusterModel
generateClusterModel(ResourceControllerDataProvider d
* Find the minimum set of replicas that need to be reassigned.
* A replica needs to be reassigned if one of the following condition is
true:
* 1. Cluster topology (the cluster config / any instance config) has been
updated.
- * 2. The baseline assignment has been updated.
- * 3. The resource config has been updated.
- * 4. The resource idealstate has been updated. TODO remove this condition
when all resource configurations are migrated to resource config.
- * 5. If the current best possible assignment does not contain the
partition's valid assignment.
+ * 2. The resource config has been updated.
+ * 3. If the current best possible assignment does not contain the
partition's valid assignment.
*
* @param replicaMap A map contains all the replicas grouped by
resource name.
* @param clusterChanges A map contains all the important metadata
updates that happened after the previous rebalance.
- * @param activeInstances All the instances that are alive and
enabled.
+ * @param activeInstances All the instances that are alive and
enabled according to the delay rebalance configuration.
+ * @param liveInstances All the instances that are alive.
* @param bestPossibleAssignment The current best possible assignment.
* @param allocatedReplicas Return the allocated replicas grouped by
the target instance name.
* @return The replicas that need to be reassigned.
*/
private static Set<AssignableReplica> findToBeAssignedReplicas(
Map<String, Set<AssignableReplica>> replicaMap,
Map<HelixConstants.ChangeType, Set<String>> clusterChanges, Set<String>
activeInstances,
- Map<String, ResourceAssignment> bestPossibleAssignment,
+ Set<String> liveInstances, Map<String, ResourceAssignment>
bestPossibleAssignment,
Map<String, Set<AssignableReplica>> allocatedReplicas) {
Set<AssignableReplica> toBeAssignedReplicas = new HashSet<>();
+
+ // newly connected nodes = changed liveInstance nodes & currently active
instances.
+ Set<String> newlyConnectedNodes = clusterChanges
+ .getOrDefault(HelixConstants.ChangeType.LIVE_INSTANCE,
Collections.emptySet());
+ newlyConnectedNodes.retainAll(liveInstances);
if (clusterChanges.containsKey(HelixConstants.ChangeType.CLUSTER_CONFIG)
|| clusterChanges
- .containsKey(HelixConstants.ChangeType.INSTANCE_CONFIG)) {
- // If the cluster topology has been modified, need to reassign all
replicas
+ .containsKey(HelixConstants.ChangeType.INSTANCE_CONFIG) ||
!newlyConnectedNodes.isEmpty()) {
+ // 1. If the cluster topology has been modified, need to reassign all
replicas.
+ // 2. If any node was re-connect, need to reassign all replicas for
balance.
Review comment:
I forgot to change the comments after renamed the parameter. Should be
"newly connected".
It's not confusing, it is for both. I guess you know too many details about
the internal implementation so you are confused. The logic here does not care
about global or partial. This is just for generically balancing the replicas
for evenness.
For the follow-up, maybe for recovery, maybe for new nodes, this logic does
not really care. New nodes, then try to move everything. That's 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:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
