policy should compare the approx val instead of actual val when moving replicas around

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

@@ -253,20 +253,7 @@ public class Policy implements MapWriter {
      * Apply the preferences and conditions
      */
     private void applyRules() {
-      if (!clusterPreferences.isEmpty()) {
+      setApproxValuesAndSortNodes(clusterPreferences, matrix);
-        //this is to set the approximate value according to the precision
-        ArrayList<Row> tmpMatrix = new ArrayList<>(matrix);
-        for (Preference p : clusterPreferences) {
-          Collections.sort(tmpMatrix, (r1, r2) -> p.compare(r1, r2, false));
-          p.setApproxVal(tmpMatrix);
-        }
-        //approximate values are set now. Let's do recursive sorting
-        Collections.sort(matrix, (Row r1, Row r2) -> {
-          int result = clusterPreferences.get(0).compare(r1, r2, true);
-          if (result == 0) result = clusterPreferences.get(0).compare(r1, r2, false);
-          return result;
-        });
-      }
 
       for (Clause clause : expandedClauses) {
         List<Violation> errs = clause.test(matrix);
@@ -274,6 +261,8 @@ public class Policy implements MapWriter {
       }
     }
 
+
+
     public List<Violation> getViolations() {
       return violations;
     }
@@ -303,6 +292,22 @@ public class Policy implements MapWriter {
     }
   }
 
+  static void setApproxValuesAndSortNodes(List<Preference> clusterPreferences, List<Row> matrix) {
+    if (!clusterPreferences.isEmpty()) {
+      //this is to set the approximate value according to the precision
+      ArrayList<Row> tmpMatrix = new ArrayList<>(matrix);
+      for (Preference p : clusterPreferences) {
+        Collections.sort(tmpMatrix, (r1, r2) -> p.compare(r1, r2, false));
+        p.setApproxVal(tmpMatrix);
+      }
+      //approximate values are set now. Let's do recursive sorting
+      Collections.sort(matrix, (Row r1, Row r2) -> {
+        int result = clusterPreferences.get(0).compare(r1, r2, true);
+        if (result == 0) result = clusterPreferences.get(0).compare(r1, r2, false);
+        return result;
+      });
+    }
+  }
 
   public Session createSession(ClusterDataProvider dataProvider) {
     return new Session(dataProvider);
@@ -470,7 +475,8 @@ public class Policy implements MapWriter {
       }
     }
 
-    protected List<Violation> testChangedMatrix(boolean strict, List<Row> rows) {
+    List<Violation> testChangedMatrix(boolean strict, List<Row> rows) {
+      setApproxValuesAndSortNodes(session.getPolicy().clusterPreferences,rows);
       List<Violation> errors = new ArrayList<>();
       for (Clause clause : session.expandedClauses) {
         if (strict || clause.strict) {
@@ -615,6 +621,6 @@ public class Policy implements MapWriter {
    * a value {@code 1} if  r1 is less loaded than r2
    */
   static int compareRows(Row r1, Row r2, Policy policy) {
-    return policy.clusterPreferences.get(0).compare(r1, r2, false);
+    return policy.clusterPreferences.get(0).compare(r1, r2, true);
   }
 }

solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy.java

@@ -19,6 +19,7 @@ package org.apache.solr.client.solrj.cloud.autoscaling;
 
 
 import java.io.IOException;
+import java.lang.invoke.MethodHandles;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -34,19 +35,24 @@ import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.cloud.autoscaling.Clause.Violation;
 import org.apache.solr.client.solrj.cloud.autoscaling.Policy.Suggester.Hint;
 import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.autoscaling.TriggerEvent;
 import org.apache.solr.common.cloud.Replica;
 import org.apache.solr.common.cloud.ReplicaPosition;
 import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CollectionParams;
 import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.common.util.Pair;
 import org.apache.solr.common.util.Utils;
 import org.apache.solr.common.util.ValidatingJsonMap;
 import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import static org.apache.solr.common.params.CollectionParams.CollectionAction.ADDREPLICA;
 import static org.apache.solr.common.params.CollectionParams.CollectionAction.MOVEREPLICA;
 
 public class TestPolicy extends SolrTestCaseJ4 {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
   public static String clusterState = "{'gettingstarted':{" +
       "    'router':{'name':'compositeId'}," +
@@ -1185,19 +1191,19 @@ public class TestPolicy extends SolrTestCaseJ4 {
 
   public void testMultiReplicaPlacement() {
     String autoScaleJson = "{" +
-        "  'cluster-preferences': [" +
+        "  cluster-preferences: [" +
         "    { maximize : freedisk , precision: 50}," +
         "    { minimize : cores, precision: 2}" +
         "  ]," +
-        "  'cluster-policy': [" +
+        "  cluster-policy: [" +
-        "    { replica : '0' , 'nodeRole': 'overseer'}," +
+        "    { replica : '0' , nodeRole: overseer}," +
-        "    { 'replica': '<2', 'shard': '#ANY', 'node': '#ANY'" +
+        "    { replica: '<2', shard: '#ANY', node: '#ANY'" +
         "    }" +
         "  ]," +
-        "  'policies': {" +
+        "  policies: {" +
-        "    'policy1': [" +
+        "    policy1: [" +
-        "      { 'replica': '<2', 'shard': '#EACH', 'node': '#ANY'}," +
+        "      { replica: '<2', shard: '#EACH', node: '#ANY'}," +
-        "      { 'replica': '<2', 'shard': '#EACH', 'sysprop.rack': 'rack1'}" +
+        "      { replica: '<2', shard: '#EACH', sysprop.rack: rack1}" +
         "    ]" +
         "  }" +
         "}";
@@ -1271,5 +1277,42 @@ public class TestPolicy extends SolrTestCaseJ4 {
     assertNull(op);
   }
 
+  public void testComputePlanAfterNodeAdded() {
+
+    String dataproviderdata = "{" +
+        "     liveNodes:[" +
+        "       '127.0.0.1:51078_solr'," +
+        "       '127.0.0.1:51147_solr']," +
+        "     replicaInfo:{" +
+        "       '127.0.0.1:51147_solr':{}," +
+        "       '127.0.0.1:51078_solr':{testNodeAdded:{shard1:[" +
+        "             { core_node3 : { type : NRT}}," +
+        "             { core_node4 : { type : NRT}}]}}}," +
+        "     nodeValues:{" +
+        "       '127.0.0.1:51147_solr':{" +
+        "         node:'127.0.0.1:51147_solr'," +
+        "         cores:0," +
+        "         freedisk : 880.5428657531738}," +
+        "       '127.0.0.1:51078_solr':{" +
+        "         node:'127.0.0.1:51078_solr'," +
+        "         cores:2," +
+        "         freedisk:880.5428695678711}}}";
+
+    String autoScalingjson = "cluster-preferences:[" +
+        "       {minimize : cores}," +
+        "       {'maximize':freedisk , precision:100}],    " +
+        " cluster-policy:[{cores:'<10',node:'#ANY'}," +
+        "       {replica:'<2', shard:'#EACH',node:'#ANY'}," +
+        "       { nodeRole:overseer,replica:0}]}";
+    Policy policy = new Policy((Map<String, Object>) Utils.fromJSONString(autoScalingjson));
+    Policy.Session session = policy.createSession(dataProviderWithData(dataproviderdata));
+    Policy.Suggester suggester = session.getSuggester(CollectionParams.CollectionAction.MOVEREPLICA)
+        .hint(Hint.TARGET_NODE, "127.0.0.1:51147_solr");
+    SolrRequest op = suggester.getOperation();
+    log.info("" + op);
+    assertNotNull(op);
+  }
+   
+
 
 }