From 6b5759efaf2e96042e247fa86b34bd9d8297abb8 Mon Sep 17 00:00:00 2001 From: Christine Poerschke Date: Fri, 6 Sep 2019 12:36:33 +0100 Subject: [PATCH] SOLR-13240: Fixed UTILIZENODE action resulting in IllegalArgumentException. (Hendrik Haddorp, Richard Goodman, Tim Owen, shalin, noble, Christine Poerschke) --- solr/CHANGES.txt | 3 + .../autoscaling/MoveReplicaSuggester.java | 9 +- .../autoscaling/MoveReplicaSuggesterTest.java | 104 ++++++++++++++++++ .../solrj/cloud/autoscaling/TestPolicy.java | 12 +- 4 files changed, 121 insertions(+), 7 deletions(-) create mode 100644 solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggesterTest.java diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 070aa0959c1..14c82761735 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -140,6 +140,9 @@ Bug Fixes * SOLR-13717: Fixed distributed grouping when multiple 'fl' params are specified (hossman, Christine Poerschke) +* SOLR-13240: Fixed UTILIZENODE action resulting in IllegalArgumentException. + (Hendrik Haddorp, Richard Goodman, Tim Owen, shalin, noble, Christine Poerschke) + Other Changes ---------------------- 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 d68f5585830..45f18b1519a 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 @@ -96,9 +96,12 @@ public class MoveReplicaSuggester extends Suggester { } static Comparator> leaderLast = (r1, r2) -> { - if (r1.first().isLeader) return 1; - if (r2.first().isLeader) return -1; - return 0; + int leaderCompare = Boolean.compare(r1.first().isLeader, r2.first().isLeader); + if ( leaderCompare != 0 ) { + return leaderCompare; + } else { + return r1.first().getName().compareTo(r2.first().getName()); + } }; diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggesterTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggesterTest.java new file mode 100644 index 00000000000..5bea6ac666a --- /dev/null +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/cloud/autoscaling/MoveReplicaSuggesterTest.java @@ -0,0 +1,104 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.client.solrj.cloud.autoscaling; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.cloud.Replica; +import org.apache.solr.common.cloud.ZkStateReader; +import org.apache.solr.common.util.Pair; +import org.junit.Test; + +import static org.apache.solr.client.solrj.cloud.autoscaling.MoveReplicaSuggester.leaderLast; + +public class MoveReplicaSuggesterTest extends SolrTestCaseJ4 { + private String CORE = "collection_shard1_replica_n1"; + private String COLLECTION = "collection"; + private String SHARD = "shard1"; + private Map IS_LEADER = Collections.singletonMap(ZkStateReader.LEADER_PROP, true); + private String NODE = "127.0.0.1:8080_solr"; + private Replica.Type REPLICA_TYPE = Replica.Type.NRT; + private Row ROW = null; + + private ReplicaInfo REPLICA_INFO_ONE = new ReplicaInfo("core_node1", CORE, COLLECTION, SHARD, REPLICA_TYPE, NODE, IS_LEADER); + private ReplicaInfo REPLICA_INFO_TWO = new ReplicaInfo("core_node2", CORE, COLLECTION, SHARD, REPLICA_TYPE, NODE, null); + private ReplicaInfo REPLICA_INFO_THREE = new ReplicaInfo("core_node3", CORE, COLLECTION, SHARD, REPLICA_TYPE, NODE, IS_LEADER); + private ReplicaInfo REPLICA_INFO_FOUR = new ReplicaInfo("core_node4", CORE, COLLECTION, SHARD, REPLICA_TYPE, NODE, null); + + private Pair PAIR_ONE = new Pair<>(REPLICA_INFO_ONE, ROW); + private Pair PAIR_TWO = new Pair<>(REPLICA_INFO_TWO, ROW); + private Pair PAIR_THREE = new Pair<>(REPLICA_INFO_THREE, ROW); + private Pair PAIR_FOUR = new Pair<>(REPLICA_INFO_FOUR, ROW); + + @Test + public void assertLeaderProperties() { + assertTrue(REPLICA_INFO_ONE.isLeader); + assertFalse(REPLICA_INFO_TWO.isLeader); + assertTrue(REPLICA_INFO_THREE.isLeader); + assertFalse(REPLICA_INFO_FOUR.isLeader); + } + + @Test + public void sortReplicasValidate() { + List> validReplicas = new ArrayList>() { + { + add(PAIR_ONE); + add(PAIR_FOUR); + add(PAIR_TWO); + } + }; + if (random().nextBoolean()) { + Collections.shuffle(validReplicas, random()); + } + validReplicas.sort(leaderLast); + + assertFalse(isReplicaLeader(validReplicas, 0)); + assertFalse(isReplicaLeader(validReplicas, 1)); + assertTrue(isReplicaLeader(validReplicas, 2)); + } + + @Test + public void sortReplicasValidateLeadersMultipleLeadersComeLast() { + List> validReplicas = new ArrayList>() { + { + add(PAIR_THREE); + add(PAIR_ONE); + add(PAIR_FOUR); + add(PAIR_TWO); + } + }; + if (random().nextBoolean()) { + Collections.shuffle(validReplicas, random()); + } + validReplicas.sort(leaderLast); + + assertFalse(isReplicaLeader(validReplicas, 0)); + assertFalse(isReplicaLeader(validReplicas, 1)); + assertTrue(isReplicaLeader(validReplicas, 2)); + assertTrue(isReplicaLeader(validReplicas, 3)); + } + + private boolean isReplicaLeader(List> replicas, int index) { + return replicas.get(index).first().isLeader; + } + +} 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 53250c093ad..921c2cbc950 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 @@ -1081,7 +1081,7 @@ public class TestPolicy extends SolrTestCaseJ4 { config = new AutoScalingConfig(policies); policy = config.getPolicy(); session = null; - for (String expectedReplica : new String[] { "r3", "r5", "r1", null }) { + for (String expectedReplica : new String[] { "r1", "r3", "r5", null }) { if (session == null) { session = policy.createSession(provider); } else { @@ -1129,8 +1129,8 @@ public class TestPolicy extends SolrTestCaseJ4 { policy = config.getPolicy(); session = null; - final String[] expectedReplica = new String[] { "r3", "r5", "r1" }; - final String[] expectedTargetNode = new String[] { "node2", "node2", "node3" }; + final String[] expectedReplica = new String[] { "r1", "r3", "r5" }; + final String[] expectedTargetNode = new String[] { "node3", "node3", "node2" }; for (int ii = 0; ii < expectedReplica.length; ++ii) { if (session == null) { session = policy.createSession(provider); @@ -2249,7 +2249,11 @@ public class TestPolicy extends SolrTestCaseJ4 { assertEquals("/c/mycoll1", l.get(0)._get( "operation/path",null)); assertNotNull(l.get(0)._get("operation/command/move-replica", null)); assertEquals("10.0.0.6:7574_solr", l.get(0)._get( "operation/command/move-replica/targetNode",null)); - assertEquals("core_node2", l.get(0)._get("operation/command/move-replica/replica", null)); + /* + * one of the two cores on 10.0.0.6:8983_solr should move to 10.0.0.6:7574_solr and + * (everything else being equal) core_node1 is chosen ahead of core_node2 based on its name + */ + assertEquals("core_node1", l.get(0)._get("operation/command/move-replica/replica", null)); }