renamed methods, added docs after review with shalin

solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientDataProvider.java

@@ -86,7 +86,7 @@ public class SolrClientDataProvider implements ClusterDataProvider, MapWriter {
   }
 
   @Override
-  public String getPolicy(String coll) {
+  public String getPolicyNameByCollection(String coll) {
     ClusterState.CollectionRef state = solrClient.getClusterStateProvider().getState(coll);
     return state == null || state.get() == null ? null : (String) state.get().getProperties().get("policy");
   }

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

@@ -46,7 +46,7 @@ class AddReplicaSuggester extends Suggester {
       Row tmpRow = row.addReplica(coll, shard);
       tmpRow.violations.clear();
 
-      List<Clause.Violation> errs = testChangedRow(strict, getModifiedMatrix(getMatrix(), tmpRow, i));
+      List<Clause.Violation> errs = testChangedMatrix(strict, getModifiedMatrix(getMatrix(), tmpRow, i));
       if(!containsNewErrors(errs)) {
         if(isLessSerious(errs, leastSeriousViolation)){
           leastSeriousViolation = errs;

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

@@ -26,7 +26,7 @@ import org.apache.solr.common.util.Utils;
 class Cell implements MapWriter {
   final int index;
   final String name;
-  Object val, val_;
+  Object val, approxVal;
 
   Cell(int index, String name, Object val) {
     this.index = index;
@@ -34,11 +34,11 @@ class Cell implements MapWriter {
     this.val = val;
   }
 
-  Cell(int index, String name, Object val, Object val_) {
+  Cell(int index, String name, Object val, Object approxVal) {
     this.index = index;
     this.name = name;
     this.val = val;
-    this.val_ = val_;
+    this.approxVal = approxVal;
   }
 
   @Override
@@ -52,6 +52,6 @@ class Cell implements MapWriter {
   }
 
   public Cell copy() {
-    return new Cell(index, name, val, val_);
+    return new Cell(index, name, val, approxVal);
   }
 }

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

@@ -28,9 +28,7 @@ import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
-import java.util.concurrent.atomic.AtomicReference;
 
-import org.apache.solr.client.solrj.request.UpdateRequest;
 import org.apache.solr.common.MapWriter;
 import org.apache.solr.common.util.Utils;
 import org.apache.solr.cloud.autoscaling.Policy.ReplicaInfo;
@@ -40,8 +38,6 @@ import static java.util.Collections.singletonMap;
 import static org.apache.solr.common.params.CoreAdminParams.COLLECTION;
 import static org.apache.solr.common.params.CoreAdminParams.REPLICA;
 import static org.apache.solr.common.params.CoreAdminParams.SHARD;
-import static org.apache.solr.cloud.autoscaling.Clause.TestStatus.FAIL;
-import static org.apache.solr.cloud.autoscaling.Clause.TestStatus.NOT_APPLICABLE;
 import static org.apache.solr.cloud.autoscaling.Clause.TestStatus.PASS;
 import static org.apache.solr.cloud.autoscaling.Operand.EQUAL;
 import static org.apache.solr.cloud.autoscaling.Operand.GREATER_THAN;
@@ -49,7 +45,6 @@ import static org.apache.solr.cloud.autoscaling.Operand.LESS_THAN;
 import static org.apache.solr.cloud.autoscaling.Operand.NOT_EQUAL;
 import static org.apache.solr.cloud.autoscaling.Operand.WILDCARD;
 import static org.apache.solr.cloud.autoscaling.Policy.ANY;
-import static org.apache.solr.cloud.autoscaling.Policy.EACH;
 
 // a set of conditions in a policy
 public class Clause implements MapWriter, Comparable<Clause> {
@@ -253,7 +248,7 @@ public class Clause implements MapWriter, Comparable<Clause> {
 
 
   public List<Violation> test(List<Row> allRows) {
-    List<Violation> errors = new ArrayList<>();
+    List<Violation> violations = new ArrayList<>();
     if (isPerCollectiontag()) {
       Map<String, Map<String, Map<String, AtomicInteger>>> replicaCount = computeReplicaCounts(allRows);
       for (Map.Entry<String, Map<String, Map<String, AtomicInteger>>> e : replicaCount.entrySet()) {
@@ -262,7 +257,7 @@ public class Clause implements MapWriter, Comparable<Clause> {
           if (!shard.isPass(shardVsCount.getKey())) continue;
           for (Map.Entry<String, AtomicInteger> counts : shardVsCount.getValue().entrySet()) {
             if (!replica.isPass(counts.getValue())) {
-              errors.add(new Violation(
+              violations.add(new Violation(
                   e.getKey(),
                   shardVsCount.getKey(),
                   tag.name.equals("node") ? counts.getKey() : null,
@@ -277,23 +272,23 @@ public class Clause implements MapWriter, Comparable<Clause> {
     } else {
       for (Row r : allRows) {
         if (!tag.isPass(r)) {
-          errors.add(new Violation(null, null, r.node, r.getVal(tag.name), tag.delta(r.getVal(tag.name)), null));
+          violations.add(new Violation(null, null, r.node, r.getVal(tag.name), tag.delta(r.getVal(tag.name)), null));
         }
       }
     }
-    return errors;
+    return violations;
 
   }
 
 
   private Map<String, Map<String, Map<String, AtomicInteger>>> computeReplicaCounts(List<Row> allRows) {
-    Map<String, Map<String, Map<String, AtomicInteger>>> replicaCount = new HashMap<>();
+    Map<String, Map<String, Map<String, AtomicInteger>>> collVsShardVsTagVsCount = new HashMap<>();
     for (Row row : allRows)
-      for (Map.Entry<String, Map<String, List<ReplicaInfo>>> colls : row.replicaInfo.entrySet()) {
+      for (Map.Entry<String, Map<String, List<ReplicaInfo>>> colls : row.collectionVsShardVsReplicas.entrySet()) {
         String collectionName = colls.getKey();
         if (!collection.isPass(collectionName)) continue;
-        replicaCount.putIfAbsent(collectionName, new HashMap<>());
+        collVsShardVsTagVsCount.putIfAbsent(collectionName, new HashMap<>());
-        Map<String, Map<String, AtomicInteger>> collMap = replicaCount.get(collectionName);
+        Map<String, Map<String, AtomicInteger>> collMap = collVsShardVsTagVsCount.get(collectionName);
         for (Map.Entry<String, List<ReplicaInfo>> shards : colls.getValue().entrySet()) {
           String shardName = shards.getKey();
           if (ANY.equals(shard.val)) shardName = ANY;
@@ -307,7 +302,7 @@ public class Clause implements MapWriter, Comparable<Clause> {
           }
         }
       }
-    return replicaCount;
+    return collVsShardVsTagVsCount;
   }
 
   public boolean isStrict() {

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

@@ -46,7 +46,7 @@ public interface ClusterDataProvider extends Closeable {
 
   Collection<String> getNodes();
 
-  String getPolicy(String coll);
+  String getPolicyNameByCollection(String coll);
 
   @Override
   default void close() throws IOException {

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

@@ -60,7 +60,7 @@ public class MoveReplicaSuggester extends Suggester {
         if (!isAllowed(targetRow.node, Hint.TARGET_NODE)) continue;
         targetRow = targetRow.addReplica(coll, shard);
         targetRow.violations.clear();
-        List<Violation> errs = testChangedRow(strict, getModifiedMatrix(getModifiedMatrix(getMatrix(), tmpRow, i), targetRow, j));
+        List<Violation> errs = testChangedMatrix(strict, getModifiedMatrix(getModifiedMatrix(getMatrix(), tmpRow, i), targetRow, j));
         if (!containsNewErrors(errs) && isLessSerious(errs, leastSeriousViolation)) {
           leastSeriousViolation = errs;
           targetNodeIndex = j;

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

@@ -178,7 +178,7 @@ public class Policy implements MapWriter {
     }
 
     private void addClausesForCollection(ClusterDataProvider dataProvider, String c) {
-      String p = dataProvider.getPolicy(c);
+      String p = dataProvider.getPolicyNameByCollection(c);
       if (p != null) {
         List<Clause> perCollPolicy = policies.get(p);
         if (perCollPolicy == null)
@@ -353,15 +353,15 @@ public class Policy implements MapWriter {
         String shard = (String) hints.get(Hint.SHARD);
         // if this is not a known collection from the existing clusterstate,
         // then add it
-        if (session.matrix.stream().noneMatch(row -> row.replicaInfo.containsKey(coll))) {
+        if (session.matrix.stream().noneMatch(row -> row.collectionVsShardVsReplicas.containsKey(coll))) {
           session.addClausesForCollection(session.dataProvider, coll);
           Collections.sort(session.expandedClauses);
         }
         if (coll != null) {
           for (Row row : session.matrix) {
-            if (!row.replicaInfo.containsKey(coll)) row.replicaInfo.put(coll, new HashMap<>());
+            if (!row.collectionVsShardVsReplicas.containsKey(coll)) row.collectionVsShardVsReplicas.put(coll, new HashMap<>());
             if (shard != null) {
-              Map<String, List<ReplicaInfo>> shardInfo = row.replicaInfo.get(coll);
+              Map<String, List<ReplicaInfo>> shardInfo = row.collectionVsShardVsReplicas.get(coll);
               if (!shardInfo.containsKey(shard)) shardInfo.put(shard, new ArrayList<>());
             }
           }
@@ -423,7 +423,7 @@ public class Policy implements MapWriter {
 
     void addReplicaToList(Row r, boolean isSource, List<Pair<Policy.ReplicaInfo, Row>> replicaList) {
       if (!isAllowed(r.node, isSource ? Hint.SRC_NODE : Hint.TARGET_NODE)) return;
-      for (Map.Entry<String, Map<String, List<Policy.ReplicaInfo>>> e : r.replicaInfo.entrySet()) {
+      for (Map.Entry<String, Map<String, List<Policy.ReplicaInfo>>> e : r.collectionVsShardVsReplicas.entrySet()) {
         if (!isAllowed(e.getKey(), Hint.COLL)) continue;
         for (Map.Entry<String, List<Policy.ReplicaInfo>> shard : e.getValue().entrySet()) {
           if (!isAllowed(e.getKey(), Hint.SHARD)) continue;
@@ -432,7 +432,7 @@ public class Policy implements MapWriter {
       }
     }
 
-    protected List<Violation> testChangedRow(boolean strict, List<Row> rows) {
+    protected List<Violation> testChangedMatrix(boolean strict, List<Row> rows) {
       List<Violation> errors = new ArrayList<>();
       for (Clause clause : session.expandedClauses) {
         if (strict || clause.strict) {

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

@@ -23,9 +23,6 @@ import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.function.BiConsumer;
-import java.util.function.Consumer;
-import java.util.function.Predicate;
 
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.common.SolrException;
@@ -33,7 +30,6 @@ import org.apache.solr.common.params.CoreAdminParams;
 import org.apache.solr.common.util.Utils;
 import org.apache.solr.cloud.autoscaling.Policy.Suggester.Hint;
 
-import static java.util.Arrays.asList;
 import static org.apache.solr.common.params.CollectionParams.CollectionAction.ADDREPLICA;
 
 public class PolicyHelper {
@@ -63,10 +59,10 @@ public class PolicyHelper {
         }
 
         @Override
-        public String getPolicy(String coll) {
+        public String getPolicyNameByCollection(String coll) {
           return optionalPolicyMapping.containsKey(coll) ?
               optionalPolicyMapping.get(coll) :
-              delegate.getPolicy(coll);
+              delegate.getPolicyNameByCollection(coll);
         }
       };
 

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

@@ -48,8 +48,8 @@ class Preference implements MapWriter {
   // recursive, it uses the precision to tie & when there is a tie use the next preference to compare
   // in non-recursive mode, precision is not taken into consideration and sort is done on actual value
   int compare(Row r1, Row r2, boolean useApprox) {
-    Object o1 = useApprox ? r1.cells[idx].val_ : r1.cells[idx].val;
+    Object o1 = useApprox ? r1.cells[idx].approxVal : r1.cells[idx].val;
-    Object o2 = useApprox ? r2.cells[idx].val_ : r2.cells[idx].val;
+    Object o2 = useApprox ? r2.cells[idx].approxVal : r2.cells[idx].val;
     int result = 0;
     if (o1 instanceof Integer && o2 instanceof Integer) result = ((Integer) o1).compareTo((Integer) o2);
     if (o1 instanceof Long && o2 instanceof Long) result = ((Long) o1).compareTo((Long) o2);
@@ -62,7 +62,7 @@ class Preference implements MapWriter {
   void setApproxVal(List<Row> tmpMatrix) {
     Object prevVal = null;
     for (Row row : tmpMatrix) {
-      prevVal = row.cells[idx].val_ =
+      prevVal = row.cells[idx].approxVal =
           prevVal == null || Math.abs(((Number) prevVal).longValue() - ((Number) row.cells[idx].val).longValue()) > precision ?
               row.cells[idx].val :
               prevVal;

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

@@ -19,7 +19,6 @@ package org.apache.solr.cloud.autoscaling;
 
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -37,16 +36,16 @@ import static org.apache.solr.common.params.CoreAdminParams.NODE;
 class Row implements MapWriter {
   public final String node;
   final Cell[] cells;
-  Map<String, Map<String, List<ReplicaInfo>>> replicaInfo;
+  Map<String, Map<String, List<ReplicaInfo>>> collectionVsShardVsReplicas;
   List<Clause> violations = new ArrayList<>();
   boolean anyValueMissing = false;
 
-  Row(String node, List<String> params, ClusterDataProvider snitch) {
+  Row(String node, List<String> params, ClusterDataProvider dataProvider) {
-    replicaInfo = snitch.getReplicaInfo(node, params);
+    collectionVsShardVsReplicas = dataProvider.getReplicaInfo(node, params);
-    if (replicaInfo == null) replicaInfo = new HashMap<>();
+    if (collectionVsShardVsReplicas == null) collectionVsShardVsReplicas = new HashMap<>();
     this.node = node;
     cells = new Cell[params.size()];
-    Map<String, Object> vals = snitch.getNodeValues(node, params);
+    Map<String, Object> vals = dataProvider.getNodeValues(node, params);
     for (int i = 0; i < params.size(); i++) {
       String s = params.get(i);
       cells[i] = new Cell(i, s, vals.get(s));
@@ -55,7 +54,7 @@ class Row implements MapWriter {
     }
   }
 
-  Row(String node, Cell[] cells, boolean anyValueMissing, Map<String, Map<String, List<ReplicaInfo>>> replicaInfo, List<Clause> violations) {
+  Row(String node, Cell[] cells, boolean anyValueMissing, Map<String, Map<String, List<ReplicaInfo>>> collectionVsShardVsReplicas, List<Clause> violations) {
     this.node = node;
     this.cells = new Cell[cells.length];
     for (int i = 0; i < this.cells.length; i++) {
@@ -63,20 +62,20 @@ class Row implements MapWriter {
 
     }
     this.anyValueMissing = anyValueMissing;
-    this.replicaInfo = replicaInfo;
+    this.collectionVsShardVsReplicas = collectionVsShardVsReplicas;
     this.violations = violations;
   }
 
   @Override
   public void writeMap(EntryWriter ew) throws IOException {
     ew.put(node, (IteratorWriter) iw -> {
-      iw.add((MapWriter) e -> e.put("replicas", replicaInfo));
+      iw.add((MapWriter) e -> e.put("replicas", collectionVsShardVsReplicas));
       for (Cell cell : cells) iw.add(cell);
     });
   }
 
   Row copy() {
-    return new Row(node, cells, anyValueMissing, Utils.getDeepCopy(replicaInfo, 3), new ArrayList<>(violations));
+    return new Row(node, cells, anyValueMissing, Utils.getDeepCopy(collectionVsShardVsReplicas, 3), new ArrayList<>(violations));
   }
 
   Object getVal(String name) {
@@ -89,10 +88,11 @@ class Row implements MapWriter {
     return node;
   }
 
+  // this adds a replica to the replica info
   Row addReplica(String coll, String shard) {
     Row row = copy();
-    Map<String, List<ReplicaInfo>> c = row.replicaInfo.get(coll);
+    Map<String, List<ReplicaInfo>> c = row.collectionVsShardVsReplicas.get(coll);
-    if (c == null) row.replicaInfo.put(coll, c = new HashMap<>());
+    if (c == null) row.collectionVsShardVsReplicas.put(coll, c = new HashMap<>());
     List<ReplicaInfo> replicas = c.get(shard);
     if (replicas == null) c.put(shard, replicas = new ArrayList<>());
     replicas.add(new ReplicaInfo("" + new Random().nextInt(1000) + 1000, coll, shard, new HashMap<>()));
@@ -105,7 +105,7 @@ class Row implements MapWriter {
 
   Pair<Row, ReplicaInfo> removeReplica(String coll, String shard) {
     Row row = copy();
-    Map<String, List<ReplicaInfo>> c = row.replicaInfo.get(coll);
+    Map<String, List<ReplicaInfo>> c = row.collectionVsShardVsReplicas.get(coll);
     if (c == null) return null;
     List<ReplicaInfo> s = c.get(shard);
     if (s == null || s.isEmpty()) return null;

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

@@ -64,10 +64,10 @@ public class TestPolicy extends SolrTestCaseJ4 {
     Row row = new Row("nodex", new Cell[]{new Cell(0, "node", "nodex")}, false, new HashMap<>(), new ArrayList<>());
     Row r1 = row.addReplica("c1", "s1");
     Row r2 = r1.addReplica("c1", "s1");
-    assertEquals(1, r1.replicaInfo.get("c1").get("s1").size());
+    assertEquals(1, r1.collectionVsShardVsReplicas.get("c1").get("s1").size());
-    assertEquals(2, r2.replicaInfo.get("c1").get("s1").size());
+    assertEquals(2, r2.collectionVsShardVsReplicas.get("c1").get("s1").size());
-    assertTrue(r2.replicaInfo.get("c1").get("s1").get(0) instanceof Policy.ReplicaInfo);
+    assertTrue(r2.collectionVsShardVsReplicas.get("c1").get("s1").get(0) instanceof Policy.ReplicaInfo);
-    assertTrue(r2.replicaInfo.get("c1").get("s1").get(1) instanceof Policy.ReplicaInfo);
+    assertTrue(r2.collectionVsShardVsReplicas.get("c1").get("s1").get(1) instanceof Policy.ReplicaInfo);
   }
 
   public void testMerge() {
@@ -252,7 +252,7 @@ public class TestPolicy extends SolrTestCaseJ4 {
       }
 
       @Override
-      public String getPolicy(String coll) {
+      public String getPolicyNameByCollection(String coll) {
         return null;
       }
     };
@@ -330,7 +330,7 @@ public class TestPolicy extends SolrTestCaseJ4 {
       }
 
       @Override
-      public String getPolicy(String coll) {
+      public String getPolicyNameByCollection(String coll) {
         return null;
       }
     });
@@ -392,7 +392,7 @@ public class TestPolicy extends SolrTestCaseJ4 {
       }
 
       @Override
-      public String getPolicy(String coll) {
+      public String getPolicyNameByCollection(String coll) {
         return "p1";
       }
     };
@@ -422,7 +422,7 @@ public class TestPolicy extends SolrTestCaseJ4 {
       }
 
       @Override
-      public String getPolicy(String coll) {
+      public String getPolicyNameByCollection(String coll) {
         return null;
       }
 
@@ -476,7 +476,7 @@ public class TestPolicy extends SolrTestCaseJ4 {
       }
 
       @Override
-      public String getPolicy(String coll) {
+      public String getPolicyNameByCollection(String coll) {
         return null;
       }