From 744d1ab974fac19f1721097f3bd5250adfbca528 Mon Sep 17 00:00:00 2001 From: Shalin Shekhar Mangar Date: Mon, 5 Jun 2017 13:41:26 +0530 Subject: [PATCH] SOLR-10782: Improve error handling and tests for Snitch and subclasses and general cleanups --- solr/CHANGES.txt | 2 + .../java/org/apache/solr/cloud/Assign.java | 11 ++-- .../OverseerCollectionMessageHandler.java | 11 +--- .../solr/cloud/rule/ServerSnitchContext.java | 18 ++----- .../handler/admin/CollectionsHandler.java | 1 - .../autoscaling/AutoScalingHandlerTest.java | 11 ++-- .../cloud/autoscaling/TestPolicyCloud.java | 22 ++++---- .../solr/cloud/rule/ImplicitSnitchTest.java | 50 +++++++++++++++++-- .../solrj/impl/SolrClientDataProvider.java | 14 ++---- .../apache/solr/cloud/autoscaling/Clause.java | 40 ++++++++------- .../apache/solr/cloud/autoscaling/Policy.java | 29 +++++------ .../solr/cloud/autoscaling/PolicyHelper.java | 2 - .../solr/cloud/autoscaling/Preference.java | 13 +++-- .../solr/common/cloud/SolrZkClient.java | 15 ------ .../common/cloud/rule/ImplicitSnitch.java | 48 +++++++++++------- .../apache/solr/common/cloud/rule/Snitch.java | 6 +-- .../solr/common/cloud/rule/SnitchContext.java | 3 +- .../org/apache/solr/common/util/Utils.java | 23 +++++++++ .../solr/cloud/autoscaling/TestPolicy.java | 8 +-- 19 files changed, 186 insertions(+), 141 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 7a3c81f1b3c..21d9ec1cd98 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -206,6 +206,8 @@ Other Changes * SOLR-10764: AutoScalingHandler should validate policy and preferences before updating zookeeper. (shalin) +* SOLR-10782: Improve error handling and tests for Snitch and subclasses and general cleanups. (Noble Paul, shalin) + ================== 6.7.0 ================== Consult the LUCENE_CHANGES.txt file for additional, low level, changes in this release. diff --git a/solr/core/src/java/org/apache/solr/cloud/Assign.java b/solr/core/src/java/org/apache/solr/cloud/Assign.java index d790e7a09bb..4e1fd68ed4e 100644 --- a/solr/core/src/java/org/apache/solr/cloud/Assign.java +++ b/solr/core/src/java/org/apache/solr/cloud/Assign.java @@ -41,19 +41,17 @@ import org.apache.solr.common.cloud.ClusterState; import org.apache.solr.common.cloud.DocCollection; import org.apache.solr.common.cloud.Replica; import org.apache.solr.common.cloud.Slice; -import org.apache.solr.common.cloud.ZkNodeProps; import org.apache.solr.common.cloud.ZkStateReader; import org.apache.solr.common.util.StrUtils; +import org.apache.solr.common.util.Utils; import org.apache.solr.core.CoreContainer; import org.apache.zookeeper.KeeperException; import static java.util.Collections.singletonMap; import static org.apache.solr.cloud.autoscaling.Policy.POLICY; -import static org.apache.solr.common.cloud.ZkStateReader.COLLECTION_PROP; import static org.apache.solr.common.cloud.ZkStateReader.CORE_NAME_PROP; import static org.apache.solr.common.cloud.ZkStateReader.MAX_SHARDS_PER_NODE; import static org.apache.solr.common.cloud.ZkStateReader.SOLR_AUTOSCALING_CONF_PATH; -import static org.apache.solr.common.params.CommonParams.NAME; public class Assign { @@ -198,8 +196,8 @@ public class Assign { positions = getNodesViaRules(clusterState, shard, numberOfNodes, cc, coll, createNodeList, l); } String policyName = coll.getStr(POLICY); - Map autoSalingJson = cc.getZkController().getZkStateReader().getZkClient().getJson(SOLR_AUTOSCALING_CONF_PATH, true); - if (policyName != null || autoSalingJson.get(Policy.CLUSTER_POLICY) != null) { + Map autoScalingJson = Utils.getJson(cc.getZkController().getZkClient(), SOLR_AUTOSCALING_CONF_PATH, true); + if (policyName != null || autoScalingJson.get(Policy.CLUSTER_POLICY) != null) { positions= Assign.getPositionsUsingPolicy(collectionName, Collections.singletonList(shard), numberOfNodes, policyName, cc.getZkController().getZkStateReader()); } @@ -223,8 +221,9 @@ public class Assign { .withClusterStateProvider(new ZkClientClusterStateProvider(zkStateReader)) .build()) { SolrClientDataProvider clientDataProvider = new SolrClientDataProvider(csc); + Map autoScalingJson = Utils.getJson(zkStateReader.getZkClient(), SOLR_AUTOSCALING_CONF_PATH, true); Map> locations = PolicyHelper.getReplicaLocations(collName, - zkStateReader.getZkClient().getJson(SOLR_AUTOSCALING_CONF_PATH, true), + autoScalingJson, clientDataProvider, singletonMap(collName, policyName), shardNames, numReplicas); Map result = new HashMap<>(); for (Map.Entry> e : locations.entrySet()) { diff --git a/solr/core/src/java/org/apache/solr/cloud/OverseerCollectionMessageHandler.java b/solr/core/src/java/org/apache/solr/cloud/OverseerCollectionMessageHandler.java index 2ff6285c399..e5b3b9b2d25 100644 --- a/solr/core/src/java/org/apache/solr/cloud/OverseerCollectionMessageHandler.java +++ b/solr/core/src/java/org/apache/solr/cloud/OverseerCollectionMessageHandler.java @@ -37,16 +37,12 @@ import com.google.common.collect.ImmutableMap; import org.apache.commons.lang.StringUtils; import org.apache.solr.client.solrj.SolrResponse; import org.apache.solr.client.solrj.SolrServerException; -import org.apache.solr.client.solrj.impl.CloudSolrClient; import org.apache.solr.client.solrj.impl.HttpSolrClient; import org.apache.solr.client.solrj.impl.HttpSolrClient.RemoteSolrException; -import org.apache.solr.client.solrj.impl.SolrClientDataProvider; -import org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider; import org.apache.solr.client.solrj.request.AbstractUpdateRequest; import org.apache.solr.client.solrj.request.UpdateRequest; import org.apache.solr.client.solrj.response.UpdateResponse; import org.apache.solr.cloud.autoscaling.Policy; -import org.apache.solr.cloud.autoscaling.PolicyHelper; import org.apache.solr.cloud.overseer.OverseerAction; import org.apache.solr.cloud.rule.ReplicaAssigner; import org.apache.solr.cloud.rule.ReplicaAssigner.Position; @@ -65,7 +61,6 @@ import org.apache.solr.common.cloud.ZkNodeProps; import org.apache.solr.common.cloud.ZkStateReader; import org.apache.solr.common.params.CollectionAdminParams; import org.apache.solr.common.params.CollectionParams.CollectionAction; -import org.apache.solr.common.params.CommonParams; import org.apache.solr.common.params.CoreAdminParams; import org.apache.solr.common.params.CoreAdminParams.CoreAdminAction; import org.apache.solr.common.params.ModifiableSolrParams; @@ -86,7 +81,6 @@ import org.apache.zookeeper.KeeperException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static java.util.Collections.singletonMap; import static org.apache.solr.cloud.autoscaling.Policy.POLICY; import static org.apache.solr.common.cloud.DocCollection.SNITCH; import static org.apache.solr.common.cloud.ZkStateReader.BASE_URL_PROP; @@ -719,8 +713,7 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler int numPullReplicas) throws KeeperException, InterruptedException { List rulesMap = (List) message.get("rule"); String policyName = message.getStr(POLICY); - Map autoSalingJson = zkStateReader.getZkClient().getJson(SOLR_AUTOSCALING_CONF_PATH, true); - autoSalingJson = autoSalingJson == null ? Collections.EMPTY_MAP : autoSalingJson; + Map autoScalingJson = Utils.getJson(zkStateReader.getZkClient(), SOLR_AUTOSCALING_CONF_PATH, true); if (rulesMap == null && policyName == null) { int i = 0; @@ -747,7 +740,7 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler } } - if (policyName != null || autoSalingJson.get(Policy.CLUSTER_POLICY) != null) { + if (policyName != null || autoScalingJson.get(Policy.CLUSTER_POLICY) != null) { return Assign.getPositionsUsingPolicy(message.getStr(COLLECTION_PROP, message.getStr(NAME)), shardNames, numNrtReplicas, policyName, zkStateReader); diff --git a/solr/core/src/java/org/apache/solr/cloud/rule/ServerSnitchContext.java b/solr/core/src/java/org/apache/solr/cloud/rule/ServerSnitchContext.java index 446c80ffc5a..01680f54443 100644 --- a/solr/core/src/java/org/apache/solr/cloud/rule/ServerSnitchContext.java +++ b/solr/core/src/java/org/apache/solr/cloud/rule/ServerSnitchContext.java @@ -19,6 +19,7 @@ package org.apache.solr.cloud.rule; import java.io.IOException; import java.lang.invoke.MethodHandles; +import java.util.Collections; import java.util.Map; import org.apache.solr.client.solrj.SolrRequest; @@ -36,7 +37,7 @@ import org.apache.solr.common.util.NamedList; import org.apache.solr.common.util.Utils; import org.apache.solr.core.CoreContainer; import org.apache.solr.update.UpdateShardHandler; -import org.apache.zookeeper.data.Stat; +import org.apache.zookeeper.KeeperException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -55,21 +56,12 @@ public class ServerSnitchContext extends SnitchContext { } - public Map getZkJson(String path) { + public Map getZkJson(String path) throws KeeperException, InterruptedException { if (coreContainer.isZooKeeperAware()) { - try { - byte[] data = coreContainer.getZkController().getZkClient().getData(path, null, new Stat(), true); - if (data == null) return null; - return (Map) Utils.fromJSON(data); - } catch (Exception e) { - log.warn("Unable to read from ZK path : " + path, e); - return null; - - } + return Utils.getJson(coreContainer.getZkController().getZkClient(), path, true); } else { - return null; + return Collections.emptyMap(); } - } public void invokeRemote(String node, ModifiableSolrParams params, String klas, RemoteCallback callback) { diff --git a/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java b/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java index fbc76a3a32a..9a3fe00fd2d 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java @@ -47,7 +47,6 @@ import org.apache.solr.cloud.OverseerSolrResponse; import org.apache.solr.cloud.OverseerTaskQueue; import org.apache.solr.cloud.OverseerTaskQueue.QueueEvent; import org.apache.solr.cloud.ZkController; -import org.apache.solr.cloud.autoscaling.Policy; import org.apache.solr.cloud.overseer.SliceMutator; import org.apache.solr.cloud.rule.ReplicaAssigner; import org.apache.solr.cloud.rule.Rule; diff --git a/solr/core/src/test/org/apache/solr/cloud/autoscaling/AutoScalingHandlerTest.java b/solr/core/src/test/org/apache/solr/cloud/autoscaling/AutoScalingHandlerTest.java index 8b0401b59fc..7bf461667b5 100644 --- a/solr/core/src/test/org/apache/solr/cloud/autoscaling/AutoScalingHandlerTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/autoscaling/AutoScalingHandlerTest.java @@ -226,7 +226,6 @@ public class AutoScalingHandlerTest extends SolrCloudTestCase { response = solrClient.request(req); assertEquals(response.get("result").toString(), "success"); -// SolrQuery query = new SolrQuery().setParam(CommonParams.QT, path); req = createAutoScalingRequest(SolrRequest.METHOD.GET, null); response = solrClient.request(req); @@ -252,17 +251,19 @@ public class AutoScalingHandlerTest extends SolrCloudTestCase { assertNotNull(sortedNodes); assertEquals(2, sortedNodes.size()); - String[] sortedNodeNames = new String[2]; for (int i = 0; i < 2; i++) { Map node = (Map) sortedNodes.get(i); assertNotNull(node); assertEquals(5, node.size()); - assertNotNull(sortedNodeNames[i] = (String) node.get("node")); + assertNotNull(node.get("node")); assertNotNull(node.get("cores")); - assertEquals("0", String.valueOf(node.get("cores"))); + assertEquals(0L, node.get("cores")); assertNotNull(node.get("freedisk")); + assertTrue(node.get("freedisk") instanceof Double); assertNotNull(node.get("sysLoadAvg")); + assertTrue(node.get("sysLoadAvg") instanceof Double); assertNotNull(node.get("heapUsage")); + assertTrue(node.get("heapUsage") instanceof Double); } List> violations = (List>) diagnostics.get("violations"); @@ -314,7 +315,7 @@ public class AutoScalingHandlerTest extends SolrCloudTestCase { static class AutoScalingRequest extends SolrRequest { protected final String message; - public AutoScalingRequest(METHOD m, String path, String message) { + AutoScalingRequest(METHOD m, String path, String message) { super(m, path); this.message = message; } diff --git a/solr/core/src/test/org/apache/solr/cloud/autoscaling/TestPolicyCloud.java b/solr/core/src/test/org/apache/solr/cloud/autoscaling/TestPolicyCloud.java index 27655e68b8c..fa592f36855 100644 --- a/solr/core/src/test/org/apache/solr/cloud/autoscaling/TestPolicyCloud.java +++ b/solr/core/src/test/org/apache/solr/cloud/autoscaling/TestPolicyCloud.java @@ -55,17 +55,19 @@ public class TestPolicyCloud extends SolrCloudTestCase { } @After - public void removeCollections() throws Exception { + public void after() throws Exception { cluster.deleteAllCollections(); + cluster.getSolrClient().getZkStateReader().getZkClient().setData(ZkStateReader.SOLR_AUTOSCALING_CONF_PATH, + "{}".getBytes(StandardCharsets.UTF_8), true); } + public void testCreateCollectionAddShardUsingPolicy() throws Exception { JettySolrRunner jetty = cluster.getRandomJetty(random()); int port = jetty.getLocalPort(); - String commands = "{set-policy :{c1 : [{replica:1 , shard:'#EACH', port: 'REPLACEPORT'}]}}".replace("REPLACEPORT",String.valueOf(port)); - Utils.fromJSONString(commands); + String commands = "{set-policy :{c1 : [{replica:1 , shard:'#EACH', port: '" + port + "'}]}}"; cluster.getSolrClient().request(AutoScalingHandlerTest.createAutoScalingRequest(SolrRequest.METHOD.POST, commands)); - Map json = cluster.getZkClient().getJson(ZkStateReader.SOLR_AUTOSCALING_CONF_PATH, true); + Map json = Utils.getJson(cluster.getZkClient(), ZkStateReader.SOLR_AUTOSCALING_CONF_PATH, true); assertEquals("full json:"+ Utils.toJSONString(json) , "#EACH", Utils.getObjectByPath(json, true, "/policies/c1[0]/shard")); CollectionAdminRequest.createCollectionWithImplicitRouter("policiesTest", null, "s1,s2", 1) @@ -80,8 +82,6 @@ public class TestPolicyCloud extends SolrCloudTestCase { coll = getCollectionState("policiesTest"); assertEquals(1, coll.getSlice("s3").getReplicas().size()); coll.getSlice("s3").forEach(replica -> assertEquals(jetty.getNodeName(), replica.getNodeName())); - cluster.getSolrClient().getZkStateReader().getZkClient().setData(ZkStateReader.SOLR_AUTOSCALING_CONF_PATH, - "{}".getBytes(StandardCharsets.UTF_8), true); } public void testDataProvider() throws IOException, SolrServerException, KeeperException, InterruptedException { @@ -98,14 +98,14 @@ public class TestPolicyCloud extends SolrCloudTestCase { assertNotNull(val.get("heapUsage")); assertNotNull(val.get("sysLoadAvg")); assertTrue(((Number) val.get("cores")).intValue() > 0); - assertTrue("freedisk value is " + ((Number) val.get("freedisk")).longValue(), ((Number) val.get("freedisk")).longValue() > 0); - assertTrue("heapUsage value is " + ((Number) val.get("heapUsage")).longValue(), ((Number) val.get("heapUsage")).longValue() > 0); - assertTrue("sysLoadAvg value is " + ((Number) val.get("sysLoadAvg")).longValue(), ((Number) val.get("sysLoadAvg")).longValue() > 0); + assertTrue("freedisk value is " + ((Number) val.get("freedisk")).doubleValue(), Double.compare(((Number) val.get("freedisk")).doubleValue(), 0.0d) > 0); + assertTrue("heapUsage value is " + ((Number) val.get("heapUsage")).doubleValue(), Double.compare(((Number) val.get("heapUsage")).doubleValue(), 0.0d) > 0); + assertTrue("sysLoadAvg value is " + ((Number) val.get("sysLoadAvg")).doubleValue(), Double.compare(((Number) val.get("sysLoadAvg")).doubleValue(), 0.0d) > 0); String overseerNode = OverseerTaskProcessor.getLeaderNode(cluster.getZkClient()); cluster.getSolrClient().request(CollectionAdminRequest.addRole(overseerNode, "overseer")); for (int i = 0; i < 10; i++) { - Map data = cluster.getSolrClient().getZkStateReader().getZkClient().getJson(ZkStateReader.ROLES, true); - if (i >= 9 && data == null) { + Map data = Utils.getJson(cluster.getZkClient(), ZkStateReader.ROLES, true); + if (i >= 9 && data.isEmpty()) { throw new RuntimeException("NO overseer node created"); } Thread.sleep(100); diff --git a/solr/core/src/test/org/apache/solr/cloud/rule/ImplicitSnitchTest.java b/solr/core/src/test/org/apache/solr/cloud/rule/ImplicitSnitchTest.java index 94ca771e544..709555f330b 100644 --- a/solr/core/src/test/org/apache/solr/cloud/rule/ImplicitSnitchTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/rule/ImplicitSnitchTest.java @@ -17,24 +17,28 @@ package org.apache.solr.cloud.rule; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import com.google.common.collect.Sets; +import org.apache.lucene.util.LuceneTestCase; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.cloud.ZkStateReader; import org.apache.solr.common.cloud.rule.ImplicitSnitch; +import org.apache.solr.common.cloud.rule.RemoteCallback; import org.apache.solr.common.cloud.rule.SnitchContext; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.zookeeper.KeeperException; import org.junit.Before; import org.junit.Test; import org.mockito.Mockito; import static org.hamcrest.core.Is.is; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.anyString; import static org.mockito.Mockito.when; -public class ImplicitSnitchTest { +public class ImplicitSnitchTest extends LuceneTestCase { private ImplicitSnitch snitch; private SnitchContext context; @@ -186,4 +190,42 @@ public class ImplicitSnitchTest { assertFalse(snitch.isKnownTag("ip_5")); } + @Test + public void testExceptions() throws Exception { + ImplicitSnitch implicitSnitch = new ImplicitSnitch(); + ServerSnitchContext noNodeExceptionSnitch = new ServerSnitchContext(null, null, new HashMap<>(), null) { + @Override + public Map getZkJson(String path) throws KeeperException, InterruptedException { + throw new KeeperException.NoNodeException(); + } + }; + implicitSnitch.getTags("", Collections.singleton(ImplicitSnitch.ROLE), noNodeExceptionSnitch); + Map map = (Map) noNodeExceptionSnitch.retrieve(ZkStateReader.ROLES); // todo it the key really supposed to /roles.json? + assertNotNull(map); + assertEquals(0, map.size()); + + implicitSnitch.getTags("", Collections.singleton(ImplicitSnitch.NODEROLE), noNodeExceptionSnitch); + map = (Map) noNodeExceptionSnitch.retrieve(ZkStateReader.ROLES); // todo it the key really supposed to /roles.json? + assertNotNull(map); + assertEquals(0, map.size()); + + ServerSnitchContext keeperExceptionSnitch = new ServerSnitchContext(null, null, new HashMap<>(), null) { + @Override + public Map getZkJson(String path) throws KeeperException, InterruptedException { + throw new KeeperException.ConnectionLossException(); + } + }; + expectThrows(SolrException.class, KeeperException.ConnectionLossException.class, () -> implicitSnitch.getTags("", Collections.singleton(ImplicitSnitch.ROLE), keeperExceptionSnitch)); + expectThrows(SolrException.class, KeeperException.ConnectionLossException.class, () -> implicitSnitch.getTags("", Collections.singleton(ImplicitSnitch.NODEROLE), keeperExceptionSnitch)); + + ServerSnitchContext remoteExceptionSnitch = new ServerSnitchContext(null, null, new HashMap<>(), null) { + @Override + public void invokeRemote(String node, ModifiableSolrParams params, String klas, RemoteCallback callback) { + throw new RuntimeException(); + } + }; + expectThrows(SolrException.class, RuntimeException.class, () -> implicitSnitch.getTags("", Collections.singleton(ImplicitSnitch.CORES), remoteExceptionSnitch)); + expectThrows(SolrException.class, RuntimeException.class, () -> implicitSnitch.getTags("", Collections.singleton(ImplicitSnitch.DISK), remoteExceptionSnitch)); + expectThrows(SolrException.class, RuntimeException.class, () -> implicitSnitch.getTags("", Collections.singleton(ImplicitSnitch.SYSPROP + "xyz"), remoteExceptionSnitch)); + } } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientDataProvider.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientDataProvider.java index 8bca7dcc3eb..e40f32b3f9d 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientDataProvider.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientDataProvider.java @@ -50,7 +50,7 @@ import org.apache.solr.common.params.SolrParams; import org.apache.solr.common.util.NamedList; import org.apache.solr.common.util.StrUtils; import org.apache.solr.common.util.Utils; -import org.apache.zookeeper.data.Stat; +import org.apache.zookeeper.KeeperException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -135,15 +135,9 @@ public class SolrClientDataProvider implements ClusterDataProvider, MapWriter { } - public Map getZkJson(String path) { - try { - byte[] data = zkClientClusterStateProvider.getZkStateReader().getZkClient().getData(path, null, new Stat(), true); - if (data == null) return null; - return (Map) Utils.fromJSON(data); - } catch (Exception e) { - log.warn("Unable to read from ZK path : " + path, e); - return null; - } + @Override + public Map getZkJson(String path) throws KeeperException, InterruptedException { + return Utils.getJson(zkClientClusterStateProvider.getZkStateReader().getZkClient(), path, true); } public void invokeRemote(String node, ModifiableSolrParams params, String klas, RemoteCallback callback) { diff --git a/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Clause.java b/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Clause.java index 1a8a7abf3bf..5e4078a4290 100644 --- a/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Clause.java +++ b/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Clause.java @@ -20,6 +20,7 @@ package org.apache.solr.cloud.autoscaling; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -36,7 +37,6 @@ import org.apache.solr.common.util.StrUtils; import org.apache.solr.common.util.Utils; import static java.util.Collections.singletonMap; -import static java.util.Collections.unmodifiableSet; 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; @@ -71,7 +71,7 @@ public class Clause implements MapWriter, Comparable { collection = parse(COLLECTION, m); shard = parse(SHARD, m); if(m.get(REPLICA) == null){ - throw new RuntimeException(StrUtils.formatString("'replica' is required" + Utils.toJSONString(m))); + throw new RuntimeException(StrUtils.formatString("'replica' is required in {0}", Utils.toJSONString(m))); } this.replica = parse(REPLICA, m); if (replica.op == WILDCARD) throw new RuntimeException("replica val cannot be null" + Utils.toJSONString(m)); @@ -344,9 +344,9 @@ public class Clause implements MapWriter, Comparable { this.type = type; this.vals = vals; this.min = min; - if(min != null && !type.isInstance(min)) throw new RuntimeException("wrong min value type"); + if(min != null && !type.isInstance(min)) throw new RuntimeException("wrong min value type, expected: " + type.getName() + " actual: " + min.getClass().getName()); this.max = max; - if(max != null && !type.isInstance(max)) throw new RuntimeException("wrong max value type"); + if(max != null && !type.isInstance(max)) throw new RuntimeException("wrong max value type, expected: " + type.getName() + " actual: " + max.getClass().getName()); } } @@ -412,13 +412,17 @@ public class Clause implements MapWriter, Comparable { } else if (val instanceof Number) { num = (Number) val; } - return num.longValue(); + + if (num != null) { + return num.longValue(); + } + throw new RuntimeException(name + ": " + val + "not a valid number"); } public static Double parseDouble(String name, Object val) { if (val == null) return null; if (val instanceof Double) return (Double) val; - Number num = 0; + Number num = null; if (val instanceof String) { try { num = Double.parseDouble((String) val); @@ -429,26 +433,28 @@ public class Clause implements MapWriter, Comparable { } else if (val instanceof Number) { num = (Number) val; } - return num.doubleValue(); + + if (num != null) { + return num.doubleValue(); + } + throw new RuntimeException(name + ": " + val + "not a valid number"); } - private static final Map validatetypes = new HashMap(); + private static final Map validatetypes = new HashMap<>(); static { validatetypes.put("collection", new ValidateInfo(String.class, null, null, null)); validatetypes.put("shard", new ValidateInfo(String.class, null, null, null)); - validatetypes.put("replica", new ValidateInfo(Long.class, null, 0l, null)); - validatetypes.put(ImplicitSnitch.PORT, new ValidateInfo(Long.class, null, 1024l, 65535l)); - validatetypes.put(ImplicitSnitch.DISK, new ValidateInfo(Long.class, null, 0l, Long.MAX_VALUE)); - validatetypes.put(ImplicitSnitch.NODEROLE, new ValidateInfo(String.class, unmodifiableSet(new HashSet(Arrays.asList("overseer"))), null, null)); - validatetypes.put(ImplicitSnitch.CORES, new ValidateInfo(Long.class, null, 0l, Long.MAX_VALUE)); + validatetypes.put("replica", new ValidateInfo(Long.class, null, 0L, null)); + validatetypes.put(ImplicitSnitch.PORT, new ValidateInfo(Long.class, null, 1L, 65535L)); + validatetypes.put(ImplicitSnitch.DISK, new ValidateInfo(Double.class, null, 0d, Double.MAX_VALUE)); + validatetypes.put(ImplicitSnitch.NODEROLE, new ValidateInfo(String.class, Collections.singleton("overseer"), null, null)); + validatetypes.put(ImplicitSnitch.CORES, new ValidateInfo(Long.class, null, 0L, Long.MAX_VALUE)); validatetypes.put(ImplicitSnitch.SYSLOADAVG, new ValidateInfo(Double.class, null, 0d, 100d)); validatetypes.put(ImplicitSnitch.HEAPUSAGE, new ValidateInfo(Double.class, null, 0d, null)); - validatetypes.put("NUMBER", new ValidateInfo(Long.class, null, 0l, Long.MAX_VALUE));//generic number validation + validatetypes.put("NUMBER", new ValidateInfo(Long.class, null, 0L, Long.MAX_VALUE));//generic number validation validatetypes.put("STRING", new ValidateInfo(String.class, null, null, null));//generic string validation validatetypes.put("node", new ValidateInfo(String.class, null, null, null)); - for (String ip : ImplicitSnitch.IP_SNITCHES) validatetypes.put(ip, new ValidateInfo(Long.class, null, 0l, 255l)); - - + for (String ip : ImplicitSnitch.IP_SNITCHES) validatetypes.put(ip, new ValidateInfo(Long.class, null, 0L, 255L)); } } diff --git a/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Policy.java b/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Policy.java index fce91470450..72aeda9f9a0 100644 --- a/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Policy.java +++ b/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Policy.java @@ -30,8 +30,8 @@ import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.function.Consumer; -import java.util.function.Predicate; +import java.util.SortedSet; +import java.util.TreeSet; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -63,11 +63,11 @@ public class Policy implements MapWriter { public static final String ANY = "#ANY"; public static final String CLUSTER_POLICY = "cluster-policy"; public static final String CLUSTER_PREFERENCE = "cluster-preferences"; - public static final Set GLOBAL_ONLY_TAGS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList("cores"))); + public static final Set GLOBAL_ONLY_TAGS = Collections.singleton("cores"); final Map> policies = new HashMap<>(); final List clusterPolicy; final List clusterPreferences; - final List params = new ArrayList<>(); + final List params; public Policy(Map jsonMap) { @@ -82,13 +82,15 @@ public class Policy implements MapWriter { if (clusterPreferences.isEmpty()) { clusterPreferences.add(new Preference((Map) Utils.fromJSONString("{minimize : cores, precision:1}"))); } + SortedSet paramsOfInterest = new TreeSet<>(); for (Preference preference : clusterPreferences) { - if (params.contains(preference.name.name())) { + if (paramsOfInterest.contains(preference.name.name())) { throw new RuntimeException(preference.name + " is repeated"); } - params.add(preference.name.toString()); - preference.idx = params.size() - 1; + paramsOfInterest.add(preference.name.toString()); } + this.params = new ArrayList<>(paramsOfInterest); + clusterPolicy = ((List>) jsonMap.getOrDefault(CLUSTER_POLICY, emptyList())).stream() .map(Clause::new) .filter(clause -> { @@ -146,16 +148,13 @@ public class Policy implements MapWriter { Set collections = new HashSet<>(); List expandedClauses; List violations = new ArrayList<>(); - private List paramsOfInterest; private Session(List nodes, ClusterDataProvider dataProvider, - List matrix, List expandedClauses, - List paramsOfInterest) { + List matrix, List expandedClauses) { this.nodes = nodes; this.dataProvider = dataProvider; this.matrix = matrix; this.expandedClauses = expandedClauses; - this.paramsOfInterest = paramsOfInterest; } Session(ClusterDataProvider dataProvider) { @@ -174,11 +173,9 @@ public class Policy implements MapWriter { } Collections.sort(expandedClauses); - List p = new ArrayList<>(params); - p.addAll(expandedClauses.stream().map(clause -> clause.tag.name).distinct().collect(Collectors.toList())); - paramsOfInterest = new ArrayList<>(p); + matrix = new ArrayList<>(nodes.size()); - for (String node : nodes) matrix.add(new Row(node, paramsOfInterest, dataProvider)); + for (String node : nodes) matrix.add(new Row(node, params, dataProvider)); applyRules(); } @@ -193,7 +190,7 @@ public class Policy implements MapWriter { } Session copy() { - return new Session(nodes, dataProvider, getMatrixCopy(), expandedClauses, paramsOfInterest); + return new Session(nodes, dataProvider, getMatrixCopy(), expandedClauses); } List getMatrixCopy() { diff --git a/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/PolicyHelper.java b/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/PolicyHelper.java index 0a82c7a9f92..168e94e0748 100644 --- a/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/PolicyHelper.java +++ b/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/PolicyHelper.java @@ -21,12 +21,10 @@ package org.apache.solr.cloud.autoscaling; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import org.apache.solr.client.solrj.SolrRequest; -import org.apache.solr.client.solrj.impl.SolrClientDataProvider; import org.apache.solr.common.SolrException; import org.apache.solr.common.params.CoreAdminParams; import org.apache.solr.common.util.Utils; diff --git a/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Preference.java b/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Preference.java index 69a9b9eefcd..60a67560fa6 100644 --- a/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Preference.java +++ b/solr/solrj/src/java/org/apache/solr/cloud/autoscaling/Preference.java @@ -57,10 +57,13 @@ class Preference implements MapWriter { 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); - if (o1 instanceof Float && o2 instanceof Float) result = ((Float) o1).compareTo((Float) o2); - if (o1 instanceof Double && o2 instanceof Double) result = ((Double) o1).compareTo((Double) o2); - return result == 0 ? next == null ? 0 : next.compare(r1, r2, useApprox) : sort.sortval * result; + else if (o1 instanceof Long && o2 instanceof Long) result = ((Long) o1).compareTo((Long) o2); + else if (o1 instanceof Float && o2 instanceof Float) result = ((Float) o1).compareTo((Float) o2); + else if (o1 instanceof Double && o2 instanceof Double) result = ((Double) o1).compareTo((Double) o2); + else if (!o1.getClass().getName().equals(o2.getClass().getName())) { + throw new RuntimeException("Unable to compare " + o1 + " of type: " + o1.getClass().getName() + " from " + r1.cells[idx].toString() + " and " + o2 + " of type: " + o2.getClass().getName() + " from " + r2.cells[idx].toString()); + } + return result == 0 ? (next == null ? 0 : next.compare(r1, r2, useApprox)) : sort.sortval * result; } //sets the new value according to precision in val_ @@ -68,7 +71,7 @@ class Preference implements MapWriter { Object prevVal = null; for (Row row : tmpMatrix) { prevVal = row.cells[idx].approxVal = - prevVal == null || Math.abs(((Number) prevVal).longValue() - ((Number) row.cells[idx].val).longValue()) > precision ? + (prevVal == null || Double.compare(Math.abs(((Number) prevVal).doubleValue() - ((Number) row.cells[idx].val).doubleValue()), precision) > 0) ? row.cells[idx].val : prevVal; } diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java b/solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java index 507f7193957..66033bc8c0d 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java @@ -32,7 +32,6 @@ import java.lang.invoke.MethodHandles; import java.nio.charset.StandardCharsets; import java.nio.file.Path; import java.util.List; -import java.util.Map; import java.util.concurrent.ExecutorService; import java.util.concurrent.RejectedExecutionException; import java.util.regex.Pattern; @@ -45,7 +44,6 @@ import org.apache.solr.common.cloud.ZkClientConnectionStrategy.ZkUpdate; import org.apache.solr.common.util.ExecutorUtil; import org.apache.solr.common.util.ObjectReleaseTracker; import org.apache.solr.common.util.SolrjNamedThreadFactory; -import org.apache.solr.common.util.Utils; import org.apache.zookeeper.CreateMode; import org.apache.zookeeper.KeeperException; import org.apache.zookeeper.KeeperException.NoNodeException; @@ -364,19 +362,6 @@ public class SolrZkClient implements Closeable { } } - public Map getJson(String path, boolean retryOnConnLoss) throws KeeperException, InterruptedException { - byte[] bytes = null; - try { - bytes = getData(path, null, null, retryOnConnLoss); - } catch (KeeperException.NoNodeException e) { - return null; - } - if (bytes != null && bytes.length > 0) { - return (Map) Utils.fromJSON(bytes); - } - return null; - } - /** * Returns node's state */ diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/rule/ImplicitSnitch.java b/solr/solrj/src/java/org/apache/solr/common/cloud/rule/ImplicitSnitch.java index a2af163c673..e88ceaff2f0 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/rule/ImplicitSnitch.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/rule/ImplicitSnitch.java @@ -28,8 +28,10 @@ import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; +import org.apache.solr.common.SolrException; import org.apache.solr.common.cloud.ZkStateReader; import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.zookeeper.KeeperException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -55,21 +57,25 @@ public class ImplicitSnitch extends Snitch { @Override public void getTags(String solrNode, Set requestedTags, SnitchContext ctx) { - if (requestedTags.contains(NODE)) ctx.getTags().put(NODE, solrNode); - if (requestedTags.contains(HOST)) { - Matcher hostAndPortMatcher = hostAndPortPattern.matcher(solrNode); - if (hostAndPortMatcher.find()) ctx.getTags().put(HOST, hostAndPortMatcher.group(1)); - } - if (requestedTags.contains(PORT)) { - Matcher hostAndPortMatcher = hostAndPortPattern.matcher(solrNode); - if (hostAndPortMatcher.find()) ctx.getTags().put(PORT, hostAndPortMatcher.group(2)); - } - if (requestedTags.contains(ROLE)) fillRole(solrNode, ctx, ROLE); - if (requestedTags.contains(NODEROLE)) fillRole(solrNode, ctx, NODEROLE);// for new policy framework + try { + if (requestedTags.contains(NODE)) ctx.getTags().put(NODE, solrNode); + if (requestedTags.contains(HOST)) { + Matcher hostAndPortMatcher = hostAndPortPattern.matcher(solrNode); + if (hostAndPortMatcher.find()) ctx.getTags().put(HOST, hostAndPortMatcher.group(1)); + } + if (requestedTags.contains(PORT)) { + Matcher hostAndPortMatcher = hostAndPortPattern.matcher(solrNode); + if (hostAndPortMatcher.find()) ctx.getTags().put(PORT, hostAndPortMatcher.group(2)); + } + if (requestedTags.contains(ROLE)) fillRole(solrNode, ctx, ROLE); + if (requestedTags.contains(NODEROLE)) fillRole(solrNode, ctx, NODEROLE);// for new policy framework - addIpTags(solrNode, requestedTags, ctx); + addIpTags(solrNode, requestedTags, ctx); - getRemoteInfo(solrNode, requestedTags, ctx); + getRemoteInfo(solrNode, requestedTags, ctx); + } catch (Exception e) { + throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e); + } } protected void getRemoteInfo(String solrNode, Set requestedTags, SnitchContext ctx) { @@ -82,16 +88,24 @@ public class ImplicitSnitch extends Snitch { if (params.size() > 0) ctx.invokeRemote(solrNode, params, "org.apache.solr.cloud.rule.ImplicitSnitch", null); } - private void fillRole(String solrNode, SnitchContext ctx, String key) { + private void fillRole(String solrNode, SnitchContext ctx, String key) throws KeeperException, InterruptedException { Map roles = (Map) ctx.retrieve(ZkStateReader.ROLES); // we don't want to hit the ZK for each node // so cache and reuse - if(roles == null) roles = ctx.getZkJson(ZkStateReader.ROLES); - ctx.store(ZkStateReader.ROLES, roles == null ? Collections.emptyMap() : roles); + try { + if (roles == null) roles = ctx.getZkJson(ZkStateReader.ROLES); + cacheRoles(solrNode, ctx, key, roles); + } catch (KeeperException.NoNodeException e) { + cacheRoles(solrNode, ctx, key, Collections.emptyMap()); + } + } + + private void cacheRoles(String solrNode, SnitchContext ctx, String key, Map roles) { + ctx.store(ZkStateReader.ROLES, roles); if (roles != null) { for (Object o : roles.entrySet()) { Map.Entry e = (Map.Entry) o; if (e.getValue() instanceof List) { - if(((List) e.getValue()).contains(solrNode)) { + if (((List) e.getValue()).contains(solrNode)) { ctx.getTags().put(key, e.getKey()); break; } diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/rule/Snitch.java b/solr/solrj/src/java/org/apache/solr/common/cloud/rule/Snitch.java index e0417a7aba1..7f9cbcd0882 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/rule/Snitch.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/rule/Snitch.java @@ -16,18 +16,14 @@ */ package org.apache.solr.common.cloud.rule; -import java.util.Arrays; import java.util.Collections; -import java.util.HashSet; import java.util.Set; -import org.apache.solr.common.cloud.rule.ImplicitSnitch; - /** * */ public abstract class Snitch { - public static final Set WELL_KNOWN_SNITCHES = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(ImplicitSnitch.class))); + public static final Set WELL_KNOWN_SNITCHES = Collections.singleton(ImplicitSnitch.class); public abstract void getTags(String solrNode, Set requestedTags, SnitchContext ctx); diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/rule/SnitchContext.java b/solr/solrj/src/java/org/apache/solr/common/cloud/rule/SnitchContext.java index 69a353ecc19..584533e8300 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/rule/SnitchContext.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/rule/SnitchContext.java @@ -22,6 +22,7 @@ import java.util.Map; import java.util.Set; import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.zookeeper.KeeperException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -58,7 +59,7 @@ public abstract class SnitchContext implements RemoteCallback { } - public abstract Map getZkJson(String path) ; + public abstract Map getZkJson(String path) throws KeeperException, InterruptedException; public String getNode() { return node; diff --git a/solr/solrj/src/java/org/apache/solr/common/util/Utils.java b/solr/solrj/src/java/org/apache/solr/common/util/Utils.java index cf83dee7b4c..5dc96f01fe0 100644 --- a/solr/solrj/src/java/org/apache/solr/common/util/Utils.java +++ b/solr/solrj/src/java/org/apache/solr/common/util/Utils.java @@ -41,6 +41,9 @@ import org.apache.http.util.EntityUtils; import org.apache.solr.common.IteratorWriter; import org.apache.solr.common.MapWriter; import org.apache.solr.common.SolrException; +import org.apache.solr.common.cloud.SolrZkClient; +import org.apache.solr.common.cloud.ZkOperation; +import org.apache.zookeeper.KeeperException; import org.noggit.CharArr; import org.noggit.JSONParser; import org.noggit.JSONWriter; @@ -267,6 +270,26 @@ public class Utils { while (is.read() != -1) {} } + /** + * Assumes data in ZooKeeper is a JSON string, deserializes it and returns as a Map + * + * @param zkClient the zookeeper client + * @param path the path to the znode being read + * @param retryOnConnLoss whether to retry the operation automatically on connection loss, see {@link org.apache.solr.common.cloud.ZkCmdExecutor#retryOperation(ZkOperation)} + * @return a Map if the node exists and contains valid JSON or an empty map if znode does not exist or has a null data + */ + public static Map getJson(SolrZkClient zkClient, String path, boolean retryOnConnLoss) throws KeeperException, InterruptedException { + try { + byte[] bytes = zkClient.getData(path, null, null, retryOnConnLoss); + if (bytes != null && bytes.length > 0) { + return (Map) Utils.fromJSON(bytes); + } + } catch (KeeperException.NoNodeException e) { + return Collections.emptyMap(); + } + return Collections.emptyMap(); + } + public static final Pattern ARRAY_ELEMENT_INDEX = Pattern .compile("(\\S*?)\\[(\\d+)\\]"); } diff --git a/solr/solrj/src/test/org/apache/solr/cloud/autoscaling/TestPolicy.java b/solr/solrj/src/test/org/apache/solr/cloud/autoscaling/TestPolicy.java index 120276cd5cc..f9921095150 100644 --- a/solr/solrj/src/test/org/apache/solr/cloud/autoscaling/TestPolicy.java +++ b/solr/solrj/src/test/org/apache/solr/cloud/autoscaling/TestPolicy.java @@ -143,8 +143,8 @@ public class TestPolicy extends SolrTestCaseJ4 { expectError("port", "70000","must be less than "); expectError("port", 70000,"must be less than "); - expectError("port", "1000","must be greater than"); - expectError("port", 1000,"must be greater than"); + expectError("port", "0","must be greater than"); + expectError("port", 0,"must be greater than"); expectError("cores", "-1","must be greater than"); @@ -268,8 +268,8 @@ public class TestPolicy extends SolrTestCaseJ4 { List l = session.getSorted(); assertEquals("node1", l.get(0).node); - assertEquals("node3", l.get(1).node); - assertEquals("node4", l.get(2).node); + assertEquals("node4", l.get(1).node); + assertEquals("node3", l.get(2).node); assertEquals("node2", l.get(3).node);