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 e325d10fc67..3a57a1fc561 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 @@ -253,14 +253,14 @@ public class Clause implements MapWriter, Comparable { new SealedClause(this, computedValueEvaluator); } - Condition parse(String s, Map m) { + Condition parse(String s, Map m) { Object expectedVal = null; ComputedType computedType = null; Object val = m.get(s); Type varType = VariableBase.getTagType(s); if (varType.meta.isHidden()) { - throw new IllegalArgumentException(formatString("''{0}'' is not allowed in a policy rule : ''{1}'' ", varType.tagName, toJSONString(m))); + throwExp(m,"''{0}'' is not allowed", varType.tagName); } try { String conditionName = s.trim(); @@ -270,8 +270,7 @@ public class Clause implements MapWriter, Comparable { expectedVal = Policy.ANY; } else if (val instanceof List) { if (!varType.meta.supportArrayVals()) { - throw new IllegalArgumentException(formatString("array values are not supported for {0} in clause {1}", - conditionName, toJSONString(m))); + throwExp(m, "array values are not supported for {0}", conditionName); } expectedVal = readListVal(m, (List) val, varType, conditionName); operand = Operand.IN; @@ -286,14 +285,12 @@ public class Clause implements MapWriter, Comparable { computedType = t; strVal = changedVal; if (varType == null || !varType.supportedComputedTypes.contains(computedType)) { - throw new IllegalArgumentException(formatString("''{0}'' is not allowed for variable : ''{1}'' , in clause : ''{2}'' ", - t, conditionName, toJSONString(m))); + throwExp(m,"''{0}'' is not allowed for variable : ''{1}''",t,conditionName); } } } if (computedType == null && ((String) val).charAt(0) == '#' && !varType.wildCards.contains(val)) { - throw new IllegalArgumentException(formatString("''{0}'' is not an allowed value for ''{1}'' , in clause : ''{2}'' . Supported value is : {3}", - val, conditionName, toJSONString(m), varType.wildCards)); + throwExp(m, "''{0}'' is not an allowed value for ''{1}'', supported value is : {2} ", val, conditionName, varType.wildCards ); } operand = varType == null ? operand : varType.getOperand(operand, strVal, computedType); @@ -309,10 +306,15 @@ public class Clause implements MapWriter, Comparable { } catch (IllegalArgumentException iae) { throw iae; } catch (Exception e) { - throw new IllegalArgumentException("Invalid tag : " + s + ":" + val, e); + throwExp(m, "Invalid tag : {0} ",s ); + return null; } } + public void throwExp(Map clause, String msg, Object... args) { + throw new IllegalArgumentException("syntax error in clause :"+ toJSONString(clause)+ " , msg: "+ formatString(msg, args)); + } + private List readListVal(Map m, List val, Type varType, String conditionName) { List list = val; list = (List) list.stream() diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/CoresVariable.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/CoresVariable.java index 4df394b7292..3344626ed42 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/CoresVariable.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/CoresVariable.java @@ -104,8 +104,9 @@ public class CoresVariable extends VariableBase { } else { return "cores: '#EQUAL' can be used only with node: '#ANY', node :[....]"; } + } else { + return ReplicaVariable.checkNonEqualOp(condition); } - return null; } @Override diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/ReplicaVariable.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/ReplicaVariable.java index 6239ec3227e..78f32b359d7 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/ReplicaVariable.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/ReplicaVariable.java @@ -88,13 +88,28 @@ class ReplicaVariable extends VariableBase { } else { return "'replica': '#EQUAL` must be used with 'node':'#ANY'"; } - } - if (condition.computedType == ComputedType.ALL) { + } else if (condition.computedType == ComputedType.ALL) { if (condition.getClause().tag != null && (condition.getClause().getTag().op == Operand.IN || condition.getClause().getTag().op == Operand.WILDCARD)) { return StrUtils.formatString("array value or wild card cannot be used for tag {0} with replica : '#ALL'", condition.getClause().tag.getName()); } + } else { + return checkNonEqualOp(condition); + } + + return null; + } + + static String checkNonEqualOp(Condition condition) { + if (condition.computedType == null && + condition.val instanceof RangeVal && + condition.op != Operand.RANGE_EQUAL) { + return "non-integer values cannot have any other operators"; + } + + if(condition.computedType == ComputedType.PERCENT && condition.op != Operand.RANGE_EQUAL){ + return "percentage values cannot have any other operators"; } return null; } 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 19db8fcd6c2..b75e1ba37f4 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 @@ -766,9 +766,7 @@ public class TestPolicy extends SolrTestCaseJ4 { assertEquals(Operand.EQUAL, REPLICA.getOperand(Operand.EQUAL, "2", null)); assertEquals(Operand.NOT_EQUAL, REPLICA.getOperand(Operand.NOT_EQUAL, "2", null)); assertEquals(Operand.RANGE_EQUAL, REPLICA.getOperand(Operand.EQUAL, "2.1", null)); - assertEquals(Operand.RANGE_NOT_EQUAL, REPLICA.getOperand(Operand.NOT_EQUAL, "2.1", null)); assertEquals(Operand.RANGE_EQUAL, REPLICA.getOperand(Operand.EQUAL, "2.01", null)); - assertEquals(Operand.RANGE_NOT_EQUAL, REPLICA.getOperand(Operand.NOT_EQUAL, "2.01", null)); Clause clause = Clause.create("{replica: '1.23', node:'#ANY'}"); assertTrue(clause.getReplica().isPass(2)); @@ -781,11 +779,9 @@ public class TestPolicy extends SolrTestCaseJ4 { assertTrue(clause.getReplica().isPass(0)); assertFalse(clause.getReplica().isPass(2)); - clause = Clause.create("{replica: '!1.23', node:'#ANY'}"); - assertFalse(clause.getReplica().isPass(2)); - assertFalse(clause.getReplica().isPass(1)); - assertTrue(clause.getReplica().isPass(0)); - assertTrue(clause.getReplica().isPass(3)); + expectThrows(IllegalArgumentException.class, + () -> Clause.create("{replica: '!1.23', node:'#ANY'}")); + clause = Clause.create("{replica: 1.23, node:'#ANY'}"); assertTrue(clause.getReplica().isPass(2)); @@ -897,6 +893,23 @@ public class TestPolicy extends SolrTestCaseJ4 { clause = Clause.create("{cores: '14%' , node:[node1, node2, node3]}"); assertEquals(Operand.IN, clause.getTag().op); + + expectThrows(IllegalArgumentException.class, + () -> Clause.create("{replica: '!14%' , node:'#ANY'}")); + + expectThrows(IllegalArgumentException.class, + () -> Clause.create("{cores: '!14%' , node:[node1, node2, node3]}")); + + expectThrows(IllegalArgumentException.class, + () -> Clause.create("{cores: '!1.66' , node:'#ANY'}")); + expectThrows(IllegalArgumentException.class, + () -> Clause.create("{replica: '<14%' , node:'#ANY'}")); + expectThrows(IllegalArgumentException.class, + () -> Clause.create("{replica: '>14%' , node:'#ANY'}")); + + expectThrows(IllegalArgumentException.class, + () -> Clause.create("{cores: '>14%' , node:'#ANY'}")); + } @@ -2437,7 +2450,7 @@ public class TestPolicy extends SolrTestCaseJ4 { " 'node':'10.0.0.6:8983_solr'," + " 'cores':2}}}"; autoScalingjson = " { cluster-policy:[" + - " { replica :'<51%',node:'#ANY' , type: TLOG } ,{ replica :'<51%',node:'#ANY' , type: PULL } ]," + + " { replica :'50%',node:'#ANY' , type: TLOG } ,{ replica :'50%',node:'#ANY' , type: PULL } ]," + " cluster-preferences :[{ minimize : cores }]}"; autoScalingConfig = new AutoScalingConfig((Map) Utils.fromJSONString(autoScalingjson)); session = autoScalingConfig.getPolicy().createSession(cloudManagerWithData(dataproviderdata));