SOLR-11026: MoveReplicaSuggester must check if the 'target' becomes more 'loaded' than the 'source' if an operation is performed

solr/CHANGES.txt

@@ -73,6 +73,9 @@ Bug Fixes
 
 * SOLR-11012: Fix three (JavaBinCodec not being closed) Resource Leak warnings. (Christine Poerschke)
 
+* SOLR-11026: MoveReplicaSuggester must check if the target becomes more 'loaded' than the source
+  if an operation is performed (noble)
+
 Optimizations
 ----------------------
 

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

@@ -46,8 +46,8 @@ public class MoveReplicaSuggester extends Suggester {
       String coll = replicaInfo.collection;
       String shard = replicaInfo.shard;
       Pair<Row, ReplicaInfo> pair = fromRow.removeReplica(coll, shard, replicaInfo.type);
-      Row tmpRow = pair.first();
+      Row srcTmpRow = pair.first();
-      if (tmpRow == null) {
+      if (srcTmpRow == null) {
         //no such replica available
         continue;
       }
@@ -58,8 +58,9 @@ public class MoveReplicaSuggester extends Suggester {
         if(!targetRow.isLive) continue;
         if (!isAllowed(targetRow.node, Hint.TARGET_NODE)) continue;
         targetRow = targetRow.addReplica(coll, shard, replicaInfo.type);
-        List<Violation> errs = testChangedMatrix(strict, getModifiedMatrix(getModifiedMatrix(getMatrix(), tmpRow, i), targetRow, j));
+        List<Violation> errs = testChangedMatrix(strict, getModifiedMatrix(getModifiedMatrix(getMatrix(), srcTmpRow, i), targetRow, j));
-        if (!containsNewErrors(errs) && isLessSerious(errs, leastSeriousViolation)) {
+        if (!containsNewErrors(errs) && isLessSerious(errs, leastSeriousViolation) &&
+            Policy.compareRows(srcTmpRow, targetRow, session.getPolicy()) < 1) {
           leastSeriousViolation = errs;
           targetNodeIndex = j;
           sourceNodeIndex = i;

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

@@ -77,9 +77,9 @@ public class Policy implements MapWriter {
   }
 
   public Policy(Map<String, Object> jsonMap) {
-
+    int[] idx = new int[1];
     List<Preference> initialClusterPreferences = ((List<Map<String, Object>>) jsonMap.getOrDefault(CLUSTER_PREFERENCES, emptyList())).stream()
-        .map(Preference::new)
+        .map(m -> new Preference(m, idx[0]++))
         .collect(toList());
     for (int i = 0; i < initialClusterPreferences.size() - 1; i++) {
       Preference preference = initialClusterPreferences.get(i);
@@ -562,4 +562,17 @@ public class Policy implements MapWriter {
   public List<String> getParams() {
     return params;
   }
+
+  /**
+   * Compares two {@link Row} loads according to a policy.
+   *
+   * @param r1 the first {@link Row} to compare
+   * @param r2 the second {@link Row} to compare
+   * @return the value {@code 0} if r1 and r2 are equally loaded
+   * a value {@code -1} if r1 is more loaded than r2
+   * 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);
+  }
 }

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

@@ -30,10 +30,15 @@ public class Preference implements MapWriter {
   Integer precision;
   final Policy.Sort sort;
   Preference next;
-  private int idx;
+  final int idx;
   private final Map original;
 
   public Preference(Map<String, Object> m) {
+    this(m, 0);
+  }
+
+  public Preference(Map<String, Object> m, int idx) {
+    this.idx = idx;
     this.original = Utils.getDeepCopy(m,3);
     sort = Policy.Sort.get(m);
     name = Policy.SortParam.get(m.get(sort.name()).toString());
@@ -43,7 +48,7 @@ public class Preference implements MapWriter {
       throw new RuntimeException("precision must be a positive value ");
     }
     if(precision< name.min || precision> name.max){
-      throw new RuntimeException(StrUtils.formatString("invalid precision value {0} must lie between {1} and {1}",
+      throw new RuntimeException(StrUtils.formatString("invalid precision value {0} , must lie between {1} and {2}",
           precision, name.min, name.max ) );
     }
 
@@ -104,4 +109,9 @@ public class Preference implements MapWriter {
   public Policy.SortParam getName() {
     return name;
   }
+
+  @Override
+  public String toString() {
+    return Utils.toJSONString(this);
+  }
 }

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

@@ -31,6 +31,7 @@ import java.util.Map;
 import com.google.common.collect.ImmutableList;
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.impl.SolrClientDataProvider;
 import org.apache.solr.client.solrj.request.CollectionAdminRequest;
 import org.apache.solr.client.solrj.cloud.autoscaling.Clause.Violation;
 import org.apache.solr.client.solrj.cloud.autoscaling.Policy.Suggester.Hint;
@@ -666,8 +667,8 @@ public class TestPolicy extends SolrTestCaseJ4 {
 
     List<Row> l = session.getSorted();
     assertEquals("node1", l.get(0).node);
-    assertEquals("node4", l.get(1).node);
+    assertEquals("node3", l.get(1).node);
-    assertEquals("node3", l.get(2).node);
+    assertEquals("node4", l.get(2).node);
     assertEquals("node2", l.get(3).node);
 
 
@@ -850,13 +851,9 @@ public class TestPolicy extends SolrTestCaseJ4 {
       }
     });
 
-    Policy.Suggester suggester = session.getSuggester(CollectionParams.CollectionAction.MOVEREPLICA)
+    Policy.Suggester suggester = session.getSuggester(MOVEREPLICA)
         .hint(Hint.TARGET_NODE, "127.0.0.1:60099_solr");
-    SolrParams op = suggester.getOperation().getParams();
+    SolrRequest op = suggester.getOperation();
-    assertNotNull(op);
-    session = suggester.getSession();
-    suggester = session.getSuggester(MOVEREPLICA).hint(Hint.TARGET_NODE, "127.0.0.1:60099_solr");
-    op = suggester.getOperation().getParams();
     assertNotNull(op);
   }
 
@@ -1045,5 +1042,39 @@ public class TestPolicy extends SolrTestCaseJ4 {
     assertTrue(locations.stream().allMatch(it -> ImmutableList.of("node2", "node1", "node3").contains(it.node)) );
   }
   
+  public void testMoveReplicaSuggester(){
+    String dataproviderdata = "{" +
+        "  'liveNodes':[" +
+        "    '10.0.0.6:7574_solr'," +
+        "    '10.0.0.6:8983_solr']," +
+        "  'replicaInfo':{" +
+        "    '10.0.0.6:7574_solr':{}," +
+        "    '10.0.0.6:8983_solr':{'mycoll1':{" +
+        "        'shard2':[{'core_node2':{'type':'NRT'}}]," +
+        "        'shard1':[{'core_node1':{'type':'NRT'}}]}}}," +
+        "  'nodeValues':{" +
+        "    '10.0.0.6:7574_solr':{" +
+        "      'node':'10.0.0.6:7574_solr'," +
+        "      'cores':0}," +
+        "    '10.0.0.6:8983_solr':{" +
+        "      'node':'10.0.0.6:8983_solr'," +
+        "      'cores':2}}}";
+    String autoScalingjson = "  '{cluster-policy':[" +
+        "    {      'cores':'<10',      'node':'#ANY'}," +
+        "    {      'replica':'<2',      'shard':'#EACH',      'node':'#ANY'}," +
+        "    {      'nodeRole':'overseer','replica':0}]," +
+        "  'cluster-preferences':[{'minimize':'cores'}]}";
+    Policy policy = new Policy((Map<String, Object>) Utils.fromJSONString(autoScalingjson));
+    Policy.Session session = policy.createSession(dataProviderWithData(dataproviderdata));
+    Policy.Suggester suggester = session.getSuggester(MOVEREPLICA).hint(Hint.TARGET_NODE, "10.0.0.6:7574_solr");
+    SolrRequest op = suggester.getOperation();
+    assertNotNull(op);
+    suggester = suggester.getSession().getSuggester(MOVEREPLICA).hint(Hint.TARGET_NODE, "10.0.0.6:7574_solr");
+    op = suggester.getOperation();
+    assertNull(op);
+
+
+  }
+
 
 }