MoveSuggester should do greedy check

solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Clause.java

@@ -217,6 +217,11 @@ public class Clause implements MapWriter, Comparable<Clause> {
     public int hashCode() {
       return hash;
     }
+    //if the delta is lower , this violation is less serious
+    public boolean isLessSerious(Violation that) {
+      return that.delta != null && delta != null &&
+          Math.abs(delta) < Math.abs(that.delta);
+    }
 
     @Override
     public boolean equals(Object that) {

solr/solrj/src/java/org/apache/solr/cloud/autoscaling/MoveReplicaSuggester.java

@@ -22,6 +22,7 @@ import java.util.List;
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.request.CollectionAdminRequest;
 import org.apache.solr.cloud.autoscaling.Clause.Violation;
+import org.apache.solr.cloud.autoscaling.Policy.ReplicaInfo;
 import org.apache.solr.cloud.autoscaling.Policy.Suggester;
 import org.apache.solr.common.util.Pair;
 
@@ -36,11 +37,16 @@ public class MoveReplicaSuggester extends Suggester {
 
   SolrRequest tryEachNode(boolean strict) {
     //iterate through elements and identify the least loaded
-    for (Pair<Policy.ReplicaInfo, Row> fromReplica : getValidReplicas(true, true, -1)) {
+    List<Clause.Violation> leastSeriousViolation = null;
+    Integer targetNodeIndex = null;
+    Integer fromNodeIndex = null;
+    ReplicaInfo fromReplicaInfo = null;
+    for (Pair<ReplicaInfo, Row> fromReplica : getValidReplicas(true, true, -1)) {
       Row fromRow = fromReplica.second();
-      String coll = fromReplica.first().collection;
+      ReplicaInfo replicaInfo = fromReplica.first();
-      String shard = fromReplica.first().shard;
+      String coll = replicaInfo.collection;
-      Pair<Row, Policy.ReplicaInfo> pair = fromRow.removeReplica(coll, shard);
+      String shard = replicaInfo.shard;
+      Pair<Row, ReplicaInfo> pair = fromRow.removeReplica(coll, shard);
       Row tmpRow = pair.first();
       if (tmpRow == null) {
         //no such replica available
@@ -55,15 +61,21 @@ public class MoveReplicaSuggester extends Suggester {
         targetRow = targetRow.addReplica(coll, shard);
         targetRow.violations.clear();
         List<Violation> errs = testChangedRow(strict, getModifiedMatrix(getModifiedMatrix(getMatrix(), tmpRow, i), targetRow, j));
-        if (!containsNewErrors(errs)) {
+        if (!containsNewErrors(errs) && isLessSerious(errs, leastSeriousViolation)) {
-          getMatrix().set(i, getMatrix().get(i).removeReplica(coll, shard).first());
+          leastSeriousViolation = errs;
-          getMatrix().set(j, getMatrix().get(j).addReplica(coll, shard));
+          targetNodeIndex = j;
+          fromNodeIndex = i;
+          fromReplicaInfo = replicaInfo;
+        }
+      }
+    }
+    if (targetNodeIndex != null && fromNodeIndex != null) {
+      getMatrix().set(fromNodeIndex, getMatrix().get(fromNodeIndex).removeReplica(fromReplicaInfo.collection, fromReplicaInfo.shard).first());
+      getMatrix().set(targetNodeIndex, getMatrix().get(targetNodeIndex).addReplica(fromReplicaInfo.collection, fromReplicaInfo.shard));
       return new CollectionAdminRequest.MoveReplica(
-              coll,
+          fromReplicaInfo.collection,
-              pair.second().name,
+          fromReplicaInfo.name,
-              targetRow.node);
+          getMatrix().get(targetNodeIndex).node);
-        }
-      }
     }
     return null;
   }

solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Policy.java

@@ -305,9 +305,11 @@ public class Policy implements MapWriter {
     public void writeMap(EntryWriter ew) throws IOException {
       ew.put(name, variables);
     }
+
     public String getCore() {
       return core;
     }
+
     public String getCollection() {
       return collection;
     }
@@ -381,6 +383,7 @@ public class Policy implements MapWriter {
 
     }
 
+    //check if the fresh set of violations is less serious than the last set of violations
     boolean isLessSerious(List<Violation> fresh, List<Violation> old) {
       if (old == null || fresh.size() < old.size()) return true;
       if (fresh.size() == old.size()) {
@@ -388,24 +391,21 @@ public class Policy implements MapWriter {
           Violation freshViolation = fresh.get(i);
           Violation oldViolation = null;
           for (Violation v : old) {
-            if(v.equals(freshViolation)){
+            if (v.equals(freshViolation)) oldViolation = v;
-              oldViolation =v;
           }
+          if (oldViolation != null && freshViolation.isLessSerious(oldViolation)) return true;
         }
-          if (oldViolation != null && oldViolation.delta != null &&
-              Math.abs(fresh.get(i).delta) < Math.abs(oldViolation.delta)) return true;
-        }
-
-      }
-      return false;
-    }
-    boolean containsNewErrors(List<Violation> errs){
-      for (Clause.Violation err : errs) {
-        if(!originalViolations.contains(err)) return true;
       }
       return false;
     }
 
+    boolean containsNewErrors(List<Violation> violations) {
+      for (Violation v : violations) {
+        int idx = originalViolations.indexOf(v);
+        if (idx < 0 || originalViolations.get(idx).isLessSerious(v)) return true;
+      }
+      return false;
+    }
 
     List<Pair<ReplicaInfo, Row>> getValidReplicas(boolean sortDesc, boolean isSource, int until) {
       List<Pair<Policy.ReplicaInfo, Row>> allPossibleReplicas = new ArrayList<>();
@@ -415,7 +415,8 @@ public class Policy implements MapWriter {
         for (int i = 0; i < until; i++) addReplicaToList(getMatrix().get(i), isSource, allPossibleReplicas);
       } else {
         if (until == -1) until = 0;
-        for (int i = getMatrix().size() - 1; i >= until; i--) addReplicaToList(getMatrix().get(i), isSource, allPossibleReplicas);
+        for (int i = getMatrix().size() - 1; i >= until; i--)
+          addReplicaToList(getMatrix().get(i), isSource, allPossibleReplicas);
       }
       return allPossibleReplicas;
     }
@@ -430,6 +431,7 @@ public class Policy implements MapWriter {
         }
       }
     }
+
     protected List<Violation> testChangedRow(boolean strict, List<Row> rows) {
       List<Violation> errors = new ArrayList<>();
       for (Clause clause : session.expandedClauses) {