From 808f934cee63fe5f4917646b065e2b7190f09499 Mon Sep 17 00:00:00 2001
From: Noble Paul <noblepaul@users.noreply.github.com>
Date: Sun, 2 Jun 2019 18:54:28 +1000
Subject: [PATCH] SOLR-13504 improve autoscaling syntax by adding a nodeset
 attribute (#693)

* SOLR-13504:  more checks and tests
---
 .../solr/client/solrj/cloud/autoscaling/Clause.java      | 8 ++------
 .../client/solrj/cloud/autoscaling/VariableBase.java     | 9 +++++++++
 .../solr/client/solrj/cloud/autoscaling/TestPolicy.java  | 8 +++++++-
 .../solr/client/solrj/cloud/autoscaling/TestPolicy2.java | 2 +-
 4 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Clause.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Clause.java
index d4ae96850b9..382e5f3df40 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Clause.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Clause.java
@@ -57,13 +57,13 @@ import static org.apache.solr.common.util.Utils.toJSONString;
  */
 public class Clause implements MapWriter, Comparable<Clause> {
   public static final String NODESET = "nodeset";
-  private static final Set<String> IGNORE_TAGS = new HashSet<>(Arrays.asList(REPLICA, COLLECTION, SHARD, "strict", "type", "put", NODESET));
+  static final Set<String> IGNORE_TAGS = new HashSet<>(Arrays.asList(REPLICA, COLLECTION, SHARD, "strict", "type", "put", NODESET));
 
   private final int hashCode;
   final boolean hasComputedValue;
   final Map<String, Object> original;
   final Clause derivedFrom;
-  private boolean nodeSetPresent = false;
+  boolean nodeSetPresent = false;
   Condition collection, shard, replica, tag, globalTag;
   final Replica.Type type;
   boolean strict;
@@ -568,10 +568,6 @@ public class Clause implements MapWriter, Comparable<Clause> {
 
   public List<Violation> test(Policy.Session session, double[] deviations) {
     if (isPerCollectiontag()) {
-      if(nodeSetPresent) {
-
-      }
-
       return tag.varType == Type.NODE ||
           (tag.varType.meta.isNodeSpecificVal() && replica.computedType == null) ?
           testPerNode(session, deviations) :
diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/VariableBase.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/VariableBase.java
index aaa874df40c..f6686ceb5f9 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/VariableBase.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/VariableBase.java
@@ -45,6 +45,15 @@ public class VariableBase implements Variable {
     }
   }
 
+  @Override
+  public String postValidate(Condition condition) {
+    if(Clause.IGNORE_TAGS.contains(condition.getName())) return null;
+    if(condition.getOperand() == Operand.WILDCARD && condition.clause.nodeSetPresent){
+      return "#EACH not supported in tags in nodeset";
+    }
+    return null;
+  }
+
   static Object getOperandAdjustedValue(Object val, Object original) {
     if (original instanceof Condition) {
       Condition condition = (Condition) original;
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy.java b/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy.java
index 9abbc547b7e..1cea2056c55 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy.java
@@ -706,6 +706,12 @@ public class TestPolicy extends SolrTestCaseJ4 {
      assertEquals(Variable.Type.NODE, clause.tag.varType);
      assertEquals(Operand.IN, clause.tag.op);
 
+    expectThrows(IllegalArgumentException.class,
+        () -> Clause.create("{replica:1, nodeset : {sysprop.zone : '#EACH'}}"));
+
+    expectThrows(IllegalArgumentException.class,
+        () -> Clause.create("{replica:1, nodeset : {host : '#EACH'}}"));
+
     expectThrows(IllegalArgumentException.class,
         () -> Clause.create("{replica:1, node: n1, nodeset : {sysprop.zone : east}}"));
 
@@ -1912,7 +1918,7 @@ public class TestPolicy extends SolrTestCaseJ4 {
           "{replica:0, nodeset:{'nodeRole':'overseer'},'strict':false}," +
           "{'replica':'<1','node':'node3'}," +
           "{'replica':'<2','node':'#ANY','shard':'#EACH'}," +
-          "{'replica':'<3','shard':'#EACH', nodeset : { 'sysprop.rack':'#EACH'}}" +
+          "{'replica':'<3','shard':'#EACH', nodeset : { 'sysprop.rack':[rack1, rack2, rack3, rack4]}}" +
           "]" +
           "}" +
           "}";
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy2.java b/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy2.java
index 6ec31cd7a49..2bceeab88e9 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy2.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/TestPolicy2.java
@@ -87,7 +87,7 @@ public class TestPolicy2 extends SolrTestCaseJ4 {
         "  'cluster-preferences':[{ minimize : cores},{maximize : freedisk, precision : 50}]}";
     if(useNodeset){
       autoScalingjson = "{cluster-policy:[" +
-          "    { replica : '<3' , shard : '#EACH', nodeset:{sysprop.zone: '#EACH'} } ]," +
+          "    { replica : '<3' , shard : '#EACH', nodeset:{sysprop.zone: [east , west]} } ]," +
           "  'cluster-preferences':[{ minimize : cores},{maximize : freedisk, precision : 50}]}";
     }
     policy = new Policy((Map<String, Object>) Utils.fromJSONString(autoScalingjson));