From 07470a07f976dab4cf0c3980c44be57066f46499 Mon Sep 17 00:00:00 2001 From: Mark Robert Miller Date: Fri, 18 Apr 2014 15:00:25 +0000 Subject: [PATCH] SOLR-5996: Fix checkIfDiffIsLegal, add some testing, refactor out a few methods into CloudInspectUtil. git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1588486 13f79535-47bb-0310-9956-ffa450edef68 --- .../cloud/FullSolrCloudDistribCmdsTest.java | 2 +- .../org/apache/solr/cloud/SyncSliceTest.java | 7 +- .../solr/cloud/TestCloudInspectUtil.java | 134 ++++++++++ .../cloud/AbstractFullDistribZkTestBase.java | 148 +---------- .../apache/solr/cloud/CloudInspectUtil.java | 231 ++++++++++++++++++ 5 files changed, 370 insertions(+), 152 deletions(-) create mode 100644 solr/core/src/test/org/apache/solr/cloud/TestCloudInspectUtil.java create mode 100644 solr/test-framework/src/java/org/apache/solr/cloud/CloudInspectUtil.java diff --git a/solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java b/solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java index 1963432e9dd..7358a696c6d 100644 --- a/solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java @@ -412,7 +412,7 @@ public class FullSolrCloudDistribCmdsTest extends AbstractFullDistribZkTestBase long cloudCount = cloudClient.query(query).getResults().getNumFound(); - compareResults(controlCount, cloudCount); + CloudInspectUtil.compareResults(controlClient, cloudClient); assertEquals("Control does not match cloud", controlCount, cloudCount); System.out.println("DOCS:" + controlCount); diff --git a/solr/core/src/test/org/apache/solr/cloud/SyncSliceTest.java b/solr/core/src/test/org/apache/solr/cloud/SyncSliceTest.java index 5d325b6dd50..553d2138e50 100644 --- a/solr/core/src/test/org/apache/solr/cloud/SyncSliceTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/SyncSliceTest.java @@ -219,11 +219,8 @@ public class SyncSliceTest extends AbstractFullDistribZkTestBase { + leaderJetty.url + " Dead Guy:" + deadJetty.url + "skip list:" + skipServers, shardFailMessage); // good place to test compareResults - boolean shouldFail = compareResults( - controlClient.query(new SolrQuery("*:*")).getResults().getNumFound(), - cloudClient.query(new SolrQuery("*:*")).getResults().getNumFound()); - assertTrue("A test that compareResults is working correctly failed", - shouldFail); + boolean shouldFail = CloudInspectUtil.compareResults(controlClient, cloudClient); + assertTrue("A test that compareResults is working correctly failed", shouldFail); jetties = new HashSet<>(); jetties.addAll(shardToJetty.get("shard1")); diff --git a/solr/core/src/test/org/apache/solr/cloud/TestCloudInspectUtil.java b/solr/core/src/test/org/apache/solr/cloud/TestCloudInspectUtil.java new file mode 100644 index 00000000000..0da90e9bf19 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/cloud/TestCloudInspectUtil.java @@ -0,0 +1,134 @@ +package org.apache.solr.cloud; + +/* + * 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. + */ + +import java.util.HashSet; +import java.util.Set; + +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.SolrDocument; +import org.apache.solr.common.SolrDocumentList; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class TestCloudInspectUtil extends SolrTestCaseJ4 { + protected static Logger log = LoggerFactory.getLogger(TestCloudInspectUtil.class); + + + @Override + @Before + public void setUp() throws Exception { + super.setUp(); + + + } + + @Override + @After + public void tearDown() throws Exception { + + super.tearDown(); + } + + @Test + public void testCheckIfDiffIsLegal() throws Exception { + Set addFails = null; + Set deleteFails = null; + SolrDocumentList a = getDocList("2", "3"); + SolrDocumentList b = getDocList("1"); + boolean legal = CloudInspectUtil.checkIfDiffIsLegal(a, b, "control", "cloud", addFails, + deleteFails); + + assertFalse(legal); + + // ################################ + + addFails = new HashSet(); + deleteFails = new HashSet(); + + a = getDocList("2", "3", "4"); + b = getDocList("2", "3"); + addFails.add("4"); + + legal = CloudInspectUtil.checkIfDiffIsLegal(a, b, "control", "cloud", addFails, + deleteFails); + + assertTrue(legal); + + // ################################ + + addFails = new HashSet(); + deleteFails = new HashSet(); + + a = getDocList("2", "3", "4"); + b = getDocList("2", "3", "5"); + addFails.add("4"); + deleteFails.add("5"); + + legal = CloudInspectUtil.checkIfDiffIsLegal(a, b, "control", "cloud", addFails, + deleteFails); + + assertTrue(legal); + + // ################################ + + addFails = new HashSet(); + deleteFails = new HashSet(); + + a = getDocList("2", "3", "4"); + b = getDocList("2", "3", "5"); + addFails.add("4"); + deleteFails.add("6"); + + legal = CloudInspectUtil.checkIfDiffIsLegal(a, b, "control", "cloud", addFails, + deleteFails); + + assertFalse(legal); + + // ################################ + + addFails = new HashSet(); + deleteFails = new HashSet(); + + a = getDocList("2", "3", "4"); + b = getDocList("2", "3", "4"); + + try { + legal = CloudInspectUtil.checkIfDiffIsLegal(a, b, "control", "cloud", + addFails, deleteFails); + fail("Expected exception because lists have no diff"); + } catch (IllegalArgumentException e) { + // expected + } + + } + + private SolrDocumentList getDocList(String ... ids) { + SolrDocumentList list = new SolrDocumentList(); + for (String id : ids) { + SolrDocument doc = new SolrDocument(); + doc.addField("id", id); + list.add(doc); + } + return list; + } + +} diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java b/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java index 70c20092156..2d8694fdf40 100644 --- a/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java +++ b/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java @@ -1052,7 +1052,7 @@ public abstract class AbstractFullDistribZkTestBase extends AbstractDistribZkTes SolrDocumentList lst1 = lastJetty.client.solrClient.query(query).getResults(); SolrDocumentList lst2 = cjetty.client.solrClient.query(query).getResults(); - showDiff(lst1, lst2, lastJetty.url, cjetty.url); + CloudInspectUtil.showDiff(lst1, lst2, lastJetty.url, cjetty.url); } } @@ -1117,92 +1117,6 @@ public abstract class AbstractFullDistribZkTestBase extends AbstractDistribZkTes } } } - - private String toStr(SolrDocumentList lst, int maxSz) { - if (lst.size() <= maxSz) return lst.toString(); - - StringBuilder sb = new StringBuilder("SolrDocumentList[sz=" + lst.size()); - if (lst.size() != lst.getNumFound()) { - sb.append(" numFound=" + lst.getNumFound()); - } - sb.append("]="); - sb.append(lst.subList(0,maxSz/2).toString()); - sb.append(" , [...] , "); - sb.append(lst.subList(lst.size()-maxSz/2, lst.size()).toString()); - - return sb.toString(); - } - - boolean checkIfDiffIsLegal(SolrDocumentList a, SolrDocumentList b, String aName, String bName, Set addFails, Set deleteFails) { - boolean legal = true; - Set setA = new HashSet<>(); - for (SolrDocument sdoc : a) { - setA.add(sdoc); - } - - Set setB = new HashSet<>(); - for (SolrDocument sdoc : b) { - setB.add(sdoc); - } - - Set onlyInA = new HashSet<>(setA); - onlyInA.removeAll(setB); - Set onlyInB = new HashSet<>(setB); - onlyInB.removeAll(setA); - - for (SolrDocument doc : onlyInA) { - if (!addFails.contains(doc.getFirstValue("id"))) { - legal = false; - } else { - System.err.println("###### Only in " + aName + ": " + onlyInA - + ", but this is expected because we found an add fail for " - + doc.getFirstValue("id")); - } - } - - for (SolrDocument doc : onlyInB) { - if (!deleteFails.contains(doc.getFirstValue("id"))) { - legal = false; - } else { - System.err.println("###### Only in " + bName + ": " + onlyInB - + ", but this is expected because we found a delete fail for " - + doc.getFirstValue("id")); - } - } - - return legal; - } - - Set showDiff(SolrDocumentList a, SolrDocumentList b, String aName, String bName) { - System.err.println("######"+aName+ ": " + toStr(a,10)); - System.err.println("######"+bName+ ": " + toStr(b,10)); - System.err.println("###### sizes=" + a.size() + "," + b.size()); - - Set setA = new HashSet<>(); - for (SolrDocument sdoc : a) { - setA.add(new HashMap(sdoc)); - } - - Set setB = new HashSet<>(); - for (SolrDocument sdoc : b) { - setB.add(new HashMap(sdoc)); - } - - Set onlyInA = new HashSet<>(setA); - onlyInA.removeAll(setB); - Set onlyInB = new HashSet<>(setB); - onlyInB.removeAll(setA); - - if (onlyInA.size() > 0) { - System.err.println("###### Only in " + aName + ": " + onlyInA); - } - if (onlyInB.size() > 0) { - System.err.println("###### Only in " + bName + ": " + onlyInB); - } - - onlyInA.addAll(onlyInB); - return onlyInA; - } /* Checks both shard replcia consistency and against the control shard. * The test will be failed if differences are found. @@ -1288,70 +1202,12 @@ public abstract class AbstractFullDistribZkTestBase extends AbstractDistribZkTes String msg = "document count mismatch. control=" + controlDocs + " sum(shards)="+ cnt + " cloudClient="+cloudClientDocs; log.error(msg); - boolean shouldFail = compareResults(controlDocs, cloudClientDocs, addFails, deleteFails); + boolean shouldFail = CloudInspectUtil.compareResults(controlClient, cloudClient, addFails, deleteFails); if (shouldFail) { fail(msg); } } } - - protected boolean compareResults(long controlDocs, long cloudClientDocs) - throws SolrServerException { - return compareResults(controlDocs, cloudClientDocs, null, null); - } - - protected boolean compareResults(long controlDocs, long cloudClientDocs, Set addFails, Set deleteFails) - throws SolrServerException { - SolrParams q; - SolrDocumentList controlDocList; - SolrDocumentList cloudDocList; - // re-execute the query getting ids - q = params("q","*:*","rows","100000", "fl","id", "tests","checkShardConsistency(vsControl)/getIds"); // add a tag to aid in debugging via logs - controlDocList = controlClient.query(q).getResults(); - if (controlDocs != controlDocList.getNumFound()) { - log.error("Something changed! control now " + controlDocList.getNumFound()); - }; - - cloudDocList = cloudClient.query(q).getResults(); - if (cloudClientDocs != cloudDocList.getNumFound()) { - log.error("Something changed! cloudClient now " + cloudDocList.getNumFound()); - }; - - if (addFails != null || deleteFails != null) { - boolean legal = checkIfDiffIsLegal(controlDocList, cloudDocList, - "controlDocList", "cloudDocList", addFails, deleteFails); - if (legal) { - return false; - } - } - - Set differences = showDiff(controlDocList, cloudDocList, - "controlDocList", "cloudDocList"); - - // get versions for the mismatched ids - boolean foundId = false; - StringBuilder ids = new StringBuilder("id:("); - for (Map doc : differences) { - ids.append(" "+doc.get("id")); - foundId = true; - } - ids.append(")"); - - if (foundId) { - // get versions for those ids that don't match - q = params("q", ids.toString(), "rows", "100000", "fl", "id,_version_", - "sort", "id asc", "tests", - "checkShardConsistency(vsControl)/getVers"); // add a tag to aid in - // debugging via logs - - SolrDocumentList a = controlClient.query(q).getResults(); - SolrDocumentList b = cloudClient.query(q).getResults(); - - log.error("controlClient :" + a + "\n\tcloudClient :" + b); - } - - return true; - } protected SolrServer getClient(String nodeName) { for (CloudJettyRunner cjetty : cloudJettys) { diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/CloudInspectUtil.java b/solr/test-framework/src/java/org/apache/solr/cloud/CloudInspectUtil.java new file mode 100644 index 00000000000..82760d431fe --- /dev/null +++ b/solr/test-framework/src/java/org/apache/solr/cloud/CloudInspectUtil.java @@ -0,0 +1,231 @@ +package org.apache.solr.cloud; + +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.client.solrj.SolrServer; +import org.apache.solr.client.solrj.SolrServerException; +import org.apache.solr.common.SolrDocument; +import org.apache.solr.common.SolrDocumentList; +import org.apache.solr.common.params.SolrParams; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/* + * 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. + */ + +public class CloudInspectUtil { + static Logger log = LoggerFactory.getLogger(CloudInspectUtil.class); + + /** + * When a and b are known to be different, this method tells if the difference + * is legal given the adds and deletes that failed from b. + * + * @param a first list of docs + * @param b second list of docs + * @param aName label for first list of docs + * @param bName label for second list of docs + * @param bAddFails null or list of the ids of adds that failed for b + * @param bDeleteFails null or list of the ids of deletes that failed for b + * @return true if the difference in a and b is legal + */ + public static boolean checkIfDiffIsLegal(SolrDocumentList a, + SolrDocumentList b, String aName, String bName, Set bAddFails, + Set bDeleteFails) { + boolean legal = true; + + Set setA = new HashSet<>(); + for (SolrDocument sdoc : a) { + setA.add(new HashMap(sdoc)); + } + + Set setB = new HashSet<>(); + for (SolrDocument sdoc : b) { + setB.add(new HashMap(sdoc)); + } + + Set onlyInA = new HashSet<>(setA); + onlyInA.removeAll(setB); + Set onlyInB = new HashSet<>(setB); + onlyInB.removeAll(setA); + + if (onlyInA.size() == 0 && onlyInB.size() == 0) { + throw new IllegalArgumentException("No difference between list a and b"); + } + + System.err.println("###### Only in " + aName + ": " + onlyInA); + System.err.println("###### Only in " + bName + ": " + onlyInB); + + for (Map doc : onlyInA) { + if (bAddFails == null || !bAddFails.contains(doc.get("id"))) { + legal = false; + // System.err.println("###### Only in " + aName + ": " + doc.get("id")); + } else { + System.err.println("###### Only in " + aName + ": " + doc.get("id") + + ", but this is expected because we found an add fail for " + + doc.get("id")); + } + } + + for (Map doc : onlyInB) { + if (bDeleteFails == null || !bDeleteFails.contains(doc.get("id"))) { + legal = false; + // System.err.println("###### Only in " + bName + ": " + doc.get("id")); + } else { + System.err.println("###### Only in " + bName + ": " + doc.get("id") + + ", but this is expected because we found a delete fail for " + + doc.get("id")); + } + } + + return legal; + } + + /** + * Shows the difference between two lists of documents. + * + * @param a the first list + * @param b the second list + * @param aName label for the first list + * @param bName label for the second list + * @return the documents only in list a + */ + public static Set showDiff(SolrDocumentList a, SolrDocumentList b, + String aName, String bName) { + System.err.println("######" + aName + ": " + toStr(a, 10)); + System.err.println("######" + bName + ": " + toStr(b, 10)); + System.err.println("###### sizes=" + a.size() + "," + b.size()); + + Set setA = new HashSet<>(); + for (SolrDocument sdoc : a) { + setA.add(new HashMap(sdoc)); + } + + Set setB = new HashSet<>(); + for (SolrDocument sdoc : b) { + setB.add(new HashMap(sdoc)); + } + + Set onlyInA = new HashSet<>(setA); + onlyInA.removeAll(setB); + Set onlyInB = new HashSet<>(setB); + onlyInB.removeAll(setA); + + if (onlyInA.size() > 0) { + System.err.println("###### Only in " + aName + ": " + onlyInA); + } + if (onlyInB.size() > 0) { + System.err.println("###### Only in " + bName + ": " + onlyInB); + } + + onlyInA.addAll(onlyInB); + return onlyInA; + } + + private static String toStr(SolrDocumentList lst, int maxSz) { + if (lst.size() <= maxSz) return lst.toString(); + + StringBuilder sb = new StringBuilder("SolrDocumentList[sz=" + lst.size()); + if (lst.size() != lst.getNumFound()) { + sb.append(" numFound=" + lst.getNumFound()); + } + sb.append("]="); + sb.append(lst.subList(0, maxSz / 2).toString()); + sb.append(" , [...] , "); + sb.append(lst.subList(lst.size() - maxSz / 2, lst.size()).toString()); + + return sb.toString(); + } + + + /** + * Compares the results of the control and cloud clients. + * + * @return true if the compared results are illegal. + */ + public static boolean compareResults(SolrServer controlServer, SolrServer cloudServer) + throws SolrServerException { + return compareResults(controlServer, cloudServer, null, null); + } + + /** + * Compares the results of the control and cloud clients. + * + * @return true if the compared results are illegal. + */ + public static boolean compareResults(SolrServer controlServer, SolrServer cloudServer, Set addFails, Set deleteFails) + throws SolrServerException { + + SolrParams q = SolrTestCaseJ4.params("q","*:*","rows","0", "tests","checkShardConsistency(vsControl)"); // add a tag to aid in debugging via logs + + SolrDocumentList controlDocList = controlServer.query(q).getResults(); + long controlDocs = controlDocList.getNumFound(); + + SolrDocumentList cloudDocList = cloudServer.query(q).getResults(); + long cloudClientDocs = cloudDocList.getNumFound(); + + // re-execute the query getting ids + q = SolrTestCaseJ4.params("q","*:*","rows","100000", "fl","id", "tests","checkShardConsistency(vsControl)/getIds"); // add a tag to aid in debugging via logs + controlDocList = controlServer.query(q).getResults(); + if (controlDocs != controlDocList.getNumFound()) { + log.error("Something changed! control now " + controlDocList.getNumFound()); + }; + + cloudDocList = cloudServer.query(q).getResults(); + if (cloudClientDocs != cloudDocList.getNumFound()) { + log.error("Something changed! cloudClient now " + cloudDocList.getNumFound()); + }; + + if (controlDocs != cloudClientDocs && (addFails != null || deleteFails != null)) { + boolean legal = CloudInspectUtil.checkIfDiffIsLegal(controlDocList, cloudDocList, + "controlDocList", "cloudDocList", addFails, deleteFails); + if (legal) { + return false; + } + } + + Set differences = CloudInspectUtil.showDiff(controlDocList, cloudDocList, + "controlDocList", "cloudDocList"); + + // get versions for the mismatched ids + boolean foundId = false; + StringBuilder ids = new StringBuilder("id:("); + for (Map doc : differences) { + ids.append(" "+doc.get("id")); + foundId = true; + } + ids.append(")"); + + if (foundId) { + // get versions for those ids that don't match + q = SolrTestCaseJ4.params("q", ids.toString(), "rows", "100000", "fl", "id,_version_", + "sort", "id asc", "tests", + "checkShardConsistency(vsControl)/getVers"); // add a tag to aid in + // debugging via logs + + SolrDocumentList a = controlServer.query(q).getResults(); + SolrDocumentList b = cloudServer.query(q).getResults(); + + log.error("controlClient :" + a + "\n\tcloudClient :" + b); + } + + return true; + } +}