From 99188ae00c0c46d9af47b9773d492de40de4aa83 Mon Sep 17 00:00:00 2001 From: yonik Date: Wed, 15 Feb 2017 22:51:21 -0500 Subject: [PATCH] SOLR-10114: add _version_ field to child documents, fix reordered-dbq to not drop child docs --- solr/CHANGES.txt | 3 + .../apache/solr/update/AddUpdateCommand.java | 3 + .../solr/update/DirectUpdateHandler2.java | 26 +++-- .../org/apache/solr/search/TestRecovery.java | 98 +++++++++++++++++-- .../java/org/apache/solr/SolrTestCaseJ4.java | 12 +++ 5 files changed, 126 insertions(+), 16 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 1842e175b69..325760272b3 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -53,6 +53,9 @@ Bug Fixes * SOLR-9837: Fix 55% performance regression of FieldCache uninvert time of numeric fields. (yonik) +* SOLR-10114: Reordered delete-by-query causes inconsistenties between shards that have + child documents (Mano Kovacs, Mihaly Toth, yonik) + Optimizations ---------------------- diff --git a/solr/core/src/java/org/apache/solr/update/AddUpdateCommand.java b/solr/core/src/java/org/apache/solr/update/AddUpdateCommand.java index db1d79b1d30..0ede72838ba 100644 --- a/solr/core/src/java/org/apache/solr/update/AddUpdateCommand.java +++ b/solr/core/src/java/org/apache/solr/update/AddUpdateCommand.java @@ -189,8 +189,11 @@ public class AddUpdateCommand extends UpdateCommand implements Iterable iw = solrCoreState.getIndexWriter(core); try { @@ -344,6 +339,10 @@ public class DirectUpdateHandler2 extends UpdateHandler implements SolrCoreState } + private Term getIdTerm(AddUpdateCommand cmd) { + return new Term(cmd.isBlock() ? "_root_" : idField.getName(), cmd.getIndexedId()); + } + private void updateDeleteTrackers(DeleteUpdateCommand cmd) { if ((cmd.getFlags() & UpdateCommand.IGNORE_AUTOCOMMIT) == 0) { if (commitWithinSoftCommit) { @@ -867,13 +866,22 @@ public class DirectUpdateHandler2 extends UpdateHandler implements SolrCoreState log.debug("updateDocValues({})", cmd); writer.updateDocValues(updateTerm, fieldsToUpdate.toArray(new Field[fieldsToUpdate.size()])); } else { + updateDocument(cmd, writer, updateTerm); + } + } + + private void updateDocument(AddUpdateCommand cmd, IndexWriter writer, Term updateTerm) throws IOException { + if(cmd.isBlock()){ + log.debug("updateDocuments({})", cmd); + writer.updateDocuments(updateTerm, cmd); + }else{ Document luceneDocument = cmd.getLuceneDocument(false); log.debug("updateDocument({})", cmd); writer.updateDocument(updateTerm, luceneDocument); } } - + ///////////////////////////////////////////////////////////////////// // SolrInfoMBean stuff: Statistics and Module Info ///////////////////////////////////////////////////////////////////// diff --git a/solr/core/src/test/org/apache/solr/search/TestRecovery.java b/solr/core/src/test/org/apache/solr/search/TestRecovery.java index 29efa523409..84ba2fa5dd7 100644 --- a/solr/core/src/test/org/apache/solr/search/TestRecovery.java +++ b/solr/core/src/test/org/apache/solr/search/TestRecovery.java @@ -23,6 +23,7 @@ import com.codahale.metrics.Gauge; import com.codahale.metrics.Meter; import com.codahale.metrics.Metric; import com.codahale.metrics.MetricRegistry; +import org.apache.solr.common.SolrInputDocument; import org.apache.solr.metrics.SolrMetricManager; import org.noggit.ObjectBuilder; @@ -69,6 +70,9 @@ public class TestRecovery extends SolrTestCaseJ4 { // TODO: fix this test to not require FSDirectory static String savedFactory; + + private interface RunnableWithException{void run () throws Exception;} + @BeforeClass public static void beforeClass() throws Exception { savedFactory = System.getProperty("solr.DirectoryFactory"); @@ -290,6 +294,86 @@ public class TestRecovery extends SolrTestCaseJ4 { @Test public void testLogReplayWithReorderedDBQ() throws Exception { + testLogReplayWithReorderedDBQWrapper(() -> { + updateJ(jsonAdd(sdoc("id", "B1", "_version_", "1010")), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + updateJ(jsonDelQ("id:B2"), params(DISTRIB_UPDATE_PARAM, FROM_LEADER, "_version_", "-1017")); // This should've arrived after the 1015th update + updateJ(jsonAdd(sdoc("id", "B2", "_version_", "1015")), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + updateJ(jsonAdd(sdoc("id", "B3", "_version_", "1020")), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + }, + () -> assertJQ(req("q", "*:*"), "/response/numFound==2") + ); + } + + @Test + public void testLogReplayWithReorderedDBQByAsterixAndChildDocs() throws Exception { + testLogReplayWithReorderedDBQWrapper(() -> { + // 1010 - will be deleted + updateJ(jsonAdd(sdocWithChildren("B1", "1010")), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + // 1018 - should be kept, including child docs + updateJ(jsonAdd(sdocWithChildren("B2", "1018")), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + // 1017 - delete should affect only 1010 + updateJ(jsonDelQ("id:*"), params(DISTRIB_UPDATE_PARAM, FROM_LEADER, "_version_", "-1017")); // This should've arrived after the 1015th update + // 1012 - will be deleted + updateJ(jsonAdd(sdoc("id", "B3", "_version_", "1012")), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + // 1020 - should be untouched + updateJ(jsonAdd(sdocWithChildren("B4", "1020")), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + }, + () -> assertJQ(req("q", "*:*"), "/response/numFound==6") + ); + } + + @Test + public void testLogReplayWithReorderedDBQByIdAndChildDocs() throws Exception { + testLogReplayWithReorderedDBQWrapper(() -> { + // 1010 - will be deleted + updateJ(jsonAdd(sdocWithChildren("B1", "1010")), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + // 1018 - should be kept, including child docs + updateJ(jsonAdd(sdocWithChildren("B2", "1018")), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + // 1017 - delete should affect only 1010 + updateJ(jsonDelQ("id:B1 id:B2 id:B3 id:B4"), params(DISTRIB_UPDATE_PARAM, FROM_LEADER, "_version_", "-1017")); // This should've arrived after the 1015th update + // 1012 - will be deleted + updateJ(jsonAdd(sdoc("id", "B3", "_version_", "1012")), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + // 1020 - should be untouched + updateJ(jsonAdd(sdocWithChildren("B4", "1020")), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + }, + () -> assertJQ(req("q", "*:*"), "/response/numFound==8") // B2, B4 and 6 children docs (delete by id does not delete child docs) + ); + } + + @Test + public void testLogReplayWithReorderedDBQInsertingChildnodes() throws Exception { + testLogReplayWithReorderedDBQWrapper(() -> { + updateJ(jsonDelQ("id:B2"), params(DISTRIB_UPDATE_PARAM, FROM_LEADER, "_version_", "-1017")); + // test doc: B1 + // 1013 - will be inserted with 3 children + updateJ(jsonAdd(sdocWithChildren("B1", "1013", 3)), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + }, + () -> assertJQ(req("q", "*:*"), "/response/numFound==4") // B1 and B2, plus 2x 3 children + ); + } + + + @Test + public void testLogReplayWithReorderedDBQUpdateWithDifferentChildCount() throws Exception { + testLogReplayWithReorderedDBQWrapper(() -> { + // control + // 1013 - will be inserted with 3 children + updateJ(jsonAdd(sdocWithChildren("B1", "1011", 2)), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + // 1018 - this should be the final + updateJ(jsonAdd(sdocWithChildren("B1", "1012", 3)), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + + // 1013 - will be inserted with 3 children + updateJ(jsonAdd(sdocWithChildren("B2", "1013", 2)), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + updateJ(jsonDelQ("id:B3"), params(DISTRIB_UPDATE_PARAM, FROM_LEADER, "_version_", "-1019")); + // 1018 - this should be the final + updateJ(jsonAdd(sdocWithChildren("B2", "1018", 3)), params(DISTRIB_UPDATE_PARAM, FROM_LEADER)); + }, + () -> assertJQ(req("q", "*:*"), "/response/numFound==8") // B1+3children+B2+3children + ); + } + + private void testLogReplayWithReorderedDBQWrapper(RunnableWithException act, RunnableWithException assrt) throws Exception { + try { DirectUpdateHandler2.commitOnClose = false; @@ -310,13 +394,11 @@ public class TestRecovery extends SolrTestCaseJ4 { clearIndex(); assertU(commit()); - updateJ(jsonAdd(sdoc("id","B1", "_version_","1010")), params(DISTRIB_UPDATE_PARAM,FROM_LEADER)); - updateJ(jsonDelQ("id:B2"), params(DISTRIB_UPDATE_PARAM,FROM_LEADER, "_version_","-1017")); // This should've arrived after the 1015th update - updateJ(jsonAdd(sdoc("id","B2", "_version_","1015")), params(DISTRIB_UPDATE_PARAM,FROM_LEADER)); - updateJ(jsonAdd(sdoc("id","B3", "_version_","1020")), params(DISTRIB_UPDATE_PARAM,FROM_LEADER)); + // Adding some documents + act.run(); - assertJQ(req("q","*:*"),"/response/numFound==0"); + assertJQ(req("q", "*:*"), "/response/numFound==0"); h.close(); createCore(); @@ -325,7 +407,7 @@ public class TestRecovery extends SolrTestCaseJ4 { // verify that previous close didn't do a commit // recovery should be blocked by our hook - assertJQ(req("q","*:*") ,"/response/numFound==0"); + assertJQ(req("q", "*:*"), "/response/numFound==0"); // unblock recovery logReplay.release(1000); @@ -333,7 +415,9 @@ public class TestRecovery extends SolrTestCaseJ4 { // wait until recovery has finished assertTrue(logReplayFinish.tryAcquire(timeout, TimeUnit.SECONDS)); - assertJQ(req("q","*:*") ,"/response/numFound==2"); + // Asserting + assrt.run(); + } finally { DirectUpdateHandler2.commitOnClose = true; UpdateLog.testing_logReplayHook = null; diff --git a/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java b/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java index 1dcd529354e..c70195cc4b0 100644 --- a/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java +++ b/solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java @@ -1296,6 +1296,18 @@ public abstract class SolrTestCaseJ4 extends LuceneTestCase { return sd; } + public SolrInputDocument sdocWithChildren(String id, String version) { + return sdocWithChildren(id, version, 2); + } + + public SolrInputDocument sdocWithChildren(String id, String version, int childCount) { + SolrInputDocument doc = sdoc("id", id, "_version_", version); + for (int i = 0; i < childCount; i++) { + doc.addChildDocument(sdoc("id", id + "_child" + i)); + } + return doc; + } + public static List sdocs(SolrInputDocument... docs) { return Arrays.asList(docs); }