From 3a6672a40e75b7c54fb2afca96e140339899226f Mon Sep 17 00:00:00 2001 From: Noble Paul Date: Tue, 18 Jul 2017 19:21:56 +0930 Subject: [PATCH] SOLR-11026: MoveReplicaSuggester must check if the 'target' becomes more 'loaded' than the 'source' if an operation is performed --- solr/CHANGES.txt | 3 ++ .../autoscaling/MoveReplicaSuggester.java | 9 ++-- .../solrj/cloud/autoscaling/Policy.java | 17 ++++++- .../solrj/cloud/autoscaling/Preference.java | 14 +++++- .../solrj/cloud/autoscaling/TestPolicy.java | 47 +++++++++++++++---- 5 files changed, 74 insertions(+), 16 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index f0e65eb717f..c9ce430884c 100644 --- a/solr/CHANGES.txt +++ b/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 ---------------------- diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggester.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggester.java index 1e0f15b84bd..b32fc989807 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggester.java +++ b/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 pair = fromRow.removeReplica(coll, shard, replicaInfo.type); - Row tmpRow = pair.first(); - if (tmpRow == null) { + Row srcTmpRow = pair.first(); + 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 errs = testChangedMatrix(strict, getModifiedMatrix(getModifiedMatrix(getMatrix(), tmpRow, i), targetRow, j)); - if (!containsNewErrors(errs) && isLessSerious(errs, leastSeriousViolation)) { + List errs = testChangedMatrix(strict, getModifiedMatrix(getModifiedMatrix(getMatrix(), srcTmpRow, i), targetRow, j)); + if (!containsNewErrors(errs) && isLessSerious(errs, leastSeriousViolation) && + Policy.compareRows(srcTmpRow, targetRow, session.getPolicy()) < 1) { leastSeriousViolation = errs; targetNodeIndex = j; sourceNodeIndex = i; diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java index 3ccf0c1c78a..62f226b57d1 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java +++ b/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 jsonMap) { - + int[] idx = new int[1]; List initialClusterPreferences = ((List>) 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 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); + } } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Preference.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Preference.java index 29245a2b487..a7fa9638e73 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Preference.java +++ b/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 m) { + this(m, 0); + } + + public Preference(Map 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); + } } 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 049cf0421ce..6e599d64d31 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 @@ -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 l = session.getSorted(); assertEquals("node1", l.get(0).node); - assertEquals("node4", l.get(1).node); - assertEquals("node3", l.get(2).node); + assertEquals("node3", l.get(1).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(); - assertNotNull(op); - session = suggester.getSession(); - suggester = session.getSuggester(MOVEREPLICA).hint(Hint.TARGET_NODE, "127.0.0.1:60099_solr"); - op = suggester.getOperation().getParams(); + SolrRequest op = suggester.getOperation(); assertNotNull(op); } @@ -1044,6 +1041,40 @@ public class TestPolicy extends SolrTestCaseJ4 { dataProvider, Collections.singletonMap("newColl", "policy1"), Arrays.asList("shard1", "shard2"), 3,0,0, null); 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) 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); + + + } }