From d545fdfd1ef462e4fb9a6a9b5678a1339e1d7d39 Mon Sep 17 00:00:00 2001 From: Robert Muir Date: Tue, 10 Sep 2013 17:19:08 +0000 Subject: [PATCH] SOLR-4909: Use DirectoryReader.openIfChanged in non-NRT mode git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1521556 13f79535-47bb-0310-9956-ffa450edef68 --- solr/CHANGES.txt | 3 + .../apache/solr/core/IndexReaderFactory.java | 2 +- .../java/org/apache/solr/core/SolrConfig.java | 4 +- .../java/org/apache/solr/core/SolrCore.java | 27 ++- .../apache/solr/update/SolrIndexConfig.java | 6 +- .../conf/bad-solrconfig-warmer-no-reopen.xml | 2 +- .../solrconfig.snippet.randomindexconfig.xml | 2 +- .../solr/core/TestArbitraryIndexDir.java | 4 +- .../org/apache/solr/core/TestNRTOpen.java | 89 +++++++++- .../org/apache/solr/core/TestNonNRTOpen.java | 154 ++++++++++++++++++ .../solr/handler/TestReplicationHandler.java | 4 +- .../solr/collection1/conf/solrconfig.xml | 8 +- .../solr/collection1/conf/solrconfig.xml | 8 +- 13 files changed, 281 insertions(+), 32 deletions(-) create mode 100644 solr/core/src/test/org/apache/solr/core/TestNonNRTOpen.java diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 360107e38a8..7e67ce331c7 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -198,6 +198,9 @@ Bug Fixes * SOLR-5215: Fix possibility of deadlock in ZooKeeper ConnectionManager. (Mark Miller, Ricardo Merizalde) +* SOLR-4909: Use DirectoryReader.openIfChanged in non-NRT mode. + (Michael Garski via Robert Muir) + Optimizations ---------------------- diff --git a/solr/core/src/java/org/apache/solr/core/IndexReaderFactory.java b/solr/core/src/java/org/apache/solr/core/IndexReaderFactory.java index 85c4d589aa5..c43310a25ad 100644 --- a/solr/core/src/java/org/apache/solr/core/IndexReaderFactory.java +++ b/solr/core/src/java/org/apache/solr/core/IndexReaderFactory.java @@ -60,7 +60,7 @@ public abstract class IndexReaderFactory implements NamedListInitializedPlugin { /** * Creates a new IndexReader instance using the given IndexWriter. *

- * This is used for opening the initial reader in NRT mode ({@code reopenReaders=true} + * This is used for opening the initial reader in NRT mode ({@code nrtMode=true} * in solrconfig.xml) * * @param writer IndexWriter diff --git a/solr/core/src/java/org/apache/solr/core/SolrConfig.java b/solr/core/src/java/org/apache/solr/core/SolrConfig.java index 2b7dd6e3477..2fe2e3c5c23 100644 --- a/solr/core/src/java/org/apache/solr/core/SolrConfig.java +++ b/solr/core/src/java/org/apache/solr/core/SolrConfig.java @@ -152,7 +152,7 @@ public class SolrConfig extends Config { defaultIndexConfig = mainIndexConfig = null; indexConfigPrefix = "indexConfig"; } - reopenReaders = getBool(indexConfigPrefix+"/reopenReaders", true); + nrtMode = getBool(indexConfigPrefix+"/nrtMode", true); // Parse indexConfig section, using mainIndex as backup in case old config is used indexConfig = new SolrIndexConfig(this, "indexConfig", mainIndexConfig); @@ -316,7 +316,7 @@ public class SolrConfig extends Config { public final int queryResultWindowSize; public final int queryResultMaxDocsCached; public final boolean enableLazyFieldLoading; - public final boolean reopenReaders; + public final boolean nrtMode; // DocSet public final float hashSetInverseLoadFactor; public final int hashDocSetMaxSize; diff --git a/solr/core/src/java/org/apache/solr/core/SolrCore.java b/solr/core/src/java/org/apache/solr/core/SolrCore.java index da8b33e9474..a80dab87e36 100644 --- a/solr/core/src/java/org/apache/solr/core/SolrCore.java +++ b/solr/core/src/java/org/apache/solr/core/SolrCore.java @@ -19,6 +19,7 @@ package org.apache.solr.core; import org.apache.commons.io.IOUtils; import org.apache.lucene.codecs.Codec; +import org.apache.lucene.index.AtomicReaderContext; import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.IndexDeletionPolicy; import org.apache.lucene.index.IndexWriter; @@ -786,10 +787,19 @@ public final class SolrCore implements SolrInfoMBean { iwRef = prev.getUpdateHandler().getSolrCoreState().getIndexWriter(null); if (iwRef != null) { final IndexWriter iw = iwRef.get(); + final SolrCore core = this; newReaderCreator = new Callable() { + // this is used during a core reload + @Override public DirectoryReader call() throws Exception { - return DirectoryReader.open(iw, true); + if(getSolrConfig().nrtMode) { + // if in NRT mode, need to open from the previous writer + return indexReaderFactory.newReader(iw, core); + } else { + // if not NRT, need to create a new reader from the directory + return indexReaderFactory.newReader(iw.getDirectory(), core); + } } }; } @@ -1355,7 +1365,7 @@ public final class SolrCore implements SolrInfoMBean { SolrIndexSearcher tmp; RefCounted newestSearcher = null; - boolean nrt = solrConfig.reopenReaders && updateHandlerReopens; + boolean nrt = solrConfig.nrtMode && updateHandlerReopens; openSearcherLock.lock(); try { @@ -1376,8 +1386,7 @@ public final class SolrCore implements SolrInfoMBean { } } - if (newestSearcher != null && solrConfig.reopenReaders - && (nrt || indexDirFile.equals(newIndexDirFile))) { + if (newestSearcher != null && (nrt || indexDirFile.equals(newIndexDirFile))) { DirectoryReader newReader; DirectoryReader currentReader = newestSearcher.get().getIndexReader(); @@ -1387,13 +1396,13 @@ public final class SolrCore implements SolrInfoMBean { RefCounted writer = getUpdateHandler().getSolrCoreState() .getIndexWriter(null); try { - if (writer != null) { - newReader = DirectoryReader.openIfChanged(currentReader, - writer.get(), true); + if (writer != null && solrConfig.nrtMode) { + // if in NRT mode, open from the writer + newReader = DirectoryReader.openIfChanged(currentReader, writer.get(), true); } else { // verbose("start reopen without writer, reader=", currentReader); + // if not in NRT mode, just re-open the reader newReader = DirectoryReader.openIfChanged(currentReader); - // verbose("reopen result", newReader); } } finally { @@ -1427,7 +1436,7 @@ public final class SolrCore implements SolrInfoMBean { DirectoryReader newReader = newReaderCreator.call(); tmp = new SolrIndexSearcher(this, newIndexDir, getLatestSchema(), getSolrConfig().indexConfig, (realtime ? "realtime":"main"), newReader, true, !realtime, true, directoryFactory); - } else if (solrConfig.reopenReaders) { + } else if (solrConfig.nrtMode) { RefCounted writer = getUpdateHandler().getSolrCoreState().getIndexWriter(this); DirectoryReader newReader = null; try { diff --git a/solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java b/solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java index 8b7b1506425..0467528b2b8 100644 --- a/solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java +++ b/solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java @@ -17,7 +17,6 @@ package org.apache.solr.update; -import org.apache.commons.io.FileUtils; import org.apache.lucene.index.*; import org.apache.lucene.index.IndexWriter.IndexReaderWarmer; import org.apache.lucene.util.InfoStream; @@ -34,7 +33,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.File; -import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.PrintStream; import java.util.List; @@ -166,8 +164,8 @@ public class SolrIndexConfig { } } mergedSegmentWarmerInfo = getPluginInfo(prefix + "/mergedSegmentWarmer", solrConfig, def.mergedSegmentWarmerInfo); - if (mergedSegmentWarmerInfo != null && solrConfig.reopenReaders == false) { - throw new IllegalArgumentException("Supplying a mergedSegmentWarmer will do nothing since reopenReaders is false"); + if (mergedSegmentWarmerInfo != null && solrConfig.nrtMode == false) { + throw new IllegalArgumentException("Supplying a mergedSegmentWarmer will do nothing since nrtMode is false"); } } diff --git a/solr/core/src/test-files/solr/collection1/conf/bad-solrconfig-warmer-no-reopen.xml b/solr/core/src/test-files/solr/collection1/conf/bad-solrconfig-warmer-no-reopen.xml index cba30ecf745..9c9c96402ec 100644 --- a/solr/core/src/test-files/solr/collection1/conf/bad-solrconfig-warmer-no-reopen.xml +++ b/solr/core/src/test-files/solr/collection1/conf/bad-solrconfig-warmer-no-reopen.xml @@ -22,6 +22,6 @@ ${tests.luceneMatchVersion:LUCENE_CURRENT} - false + false diff --git a/solr/core/src/test-files/solr/collection1/conf/solrconfig.snippet.randomindexconfig.xml b/solr/core/src/test-files/solr/collection1/conf/solrconfig.snippet.randomindexconfig.xml index 54b127c3f44..055f3d7faeb 100644 --- a/solr/core/src/test-files/solr/collection1/conf/solrconfig.snippet.randomindexconfig.xml +++ b/solr/core/src/test-files/solr/collection1/conf/solrconfig.snippet.randomindexconfig.xml @@ -35,7 +35,7 @@ A solrconfig.xml snippet containing indexConfig settings for randomized testing. ${solr.tests.ramBufferSizeMB} - ${solr.tests.reopenReaders:true} + ${solr.tests.nrtMode:true} 1000 10000 diff --git a/solr/core/src/test/org/apache/solr/core/TestArbitraryIndexDir.java b/solr/core/src/test/org/apache/solr/core/TestArbitraryIndexDir.java index 763cddfe8e4..d16456efcef 100644 --- a/solr/core/src/test/org/apache/solr/core/TestArbitraryIndexDir.java +++ b/solr/core/src/test/org/apache/solr/core/TestArbitraryIndexDir.java @@ -54,14 +54,14 @@ public class TestArbitraryIndexDir extends AbstractSolrTestCase{ @BeforeClass public static void beforeClass() { // this test wants to start solr, and then open a separate indexwriter of its own on the same dir. - System.setProperty("solr.tests.reopenReaders", "false"); + System.setProperty("solr.tests.nrtMode", "false"); System.setProperty("enable.update.log", "false"); // schema12 doesn't support _version_ savedFactory = System.getProperty("solr.DirectoryFactory"); System.setProperty("solr.directoryFactory", "org.apache.solr.core.MockFSDirectoryFactory"); } @AfterClass public static void afterClass() { - System.clearProperty("solr.tests.reopenReaders"); + System.clearProperty("solr.tests.nrtMode"); if (savedFactory == null) { System.clearProperty("solr.directoryFactory"); } else { diff --git a/solr/core/src/test/org/apache/solr/core/TestNRTOpen.java b/solr/core/src/test/org/apache/solr/core/TestNRTOpen.java index 9daff878bd6..e18e15a308d 100644 --- a/solr/core/src/test/org/apache/solr/core/TestNRTOpen.java +++ b/solr/core/src/test/org/apache/solr/core/TestNRTOpen.java @@ -18,7 +18,12 @@ package org.apache.solr.core; */ import java.io.File; +import java.util.Collections; +import java.util.HashSet; +import java.util.IdentityHashMap; +import java.util.Set; +import org.apache.lucene.index.AtomicReaderContext; import org.apache.lucene.index.DirectoryReader; import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.search.SolrIndexSearcher; @@ -34,6 +39,9 @@ public class TestNRTOpen extends SolrTestCaseJ4 { System.setProperty("solr.directoryFactory", "solr.StandardDirectoryFactory"); // and dont delete it initially System.setProperty("solr.test.leavedatadir", "true"); + // set these so that merges won't break the test + System.setProperty("solr.tests.maxBufferedDocs", "100000"); + System.setProperty("solr.tests.mergePolicy", "org.apache.lucene.index.LogDocMergePolicy"); initCore("solrconfig-basic.xml", "schema-minimal.xml"); // add a doc assertU(adoc("foo", "bar")); @@ -43,6 +51,8 @@ public class TestNRTOpen extends SolrTestCaseJ4 { // boot up again over the same index dataDir = myDir; initCore("solrconfig-basic.xml", "schema-minimal.xml"); + // startup + assertNRT(1); } @AfterClass @@ -50,23 +60,94 @@ public class TestNRTOpen extends SolrTestCaseJ4 { // ensure we clean up after ourselves, this will fire before superclass... System.clearProperty("solr.test.leavedatadir"); System.clearProperty("solr.directoryFactory"); + System.clearProperty("solr.tests.maxBufferedDocs"); + System.clearProperty("solr.tests.mergePolicy"); + } + + public void setUp() throws Exception { + super.setUp(); + // delete all, then add initial doc + assertU(delQ("*:*")); + assertU(adoc("foo", "bar")); + assertU(commit()); } public void testReaderIsNRT() { - assertNRT(); + // core reload String core = h.getCore().getName(); h.getCoreContainer().reload(core); - assertNRT(); + assertNRT(1); + + // add a doc and soft commit + assertU(adoc("baz", "doc")); + assertU(commit("softCommit", "true")); + assertNRT(2); + + // add a doc and hard commit + assertU(adoc("bazz", "doc")); + assertU(commit()); + assertNRT(3); + + // add a doc and core reload + assertU(adoc("bazzz", "doc2")); + h.getCoreContainer().reload(core); + assertNRT(4); } - private void assertNRT() { + public void testSharedCores() { + // clear out any junk + assertU(optimize()); + + Set s1 = getCoreCacheKeys(); + assertEquals(1, s1.size()); + + // add a doc, will go in a new segment + assertU(adoc("baz", "doc")); + assertU(commit("softCommit", "true")); + + Set s2 = getCoreCacheKeys(); + assertEquals(2, s2.size()); + assertTrue(s2.containsAll(s1)); + + // add two docs, will go in a new segment + assertU(adoc("foo", "doc")); + assertU(adoc("foo2", "doc")); + assertU(commit()); + + Set s3 = getCoreCacheKeys(); + assertEquals(3, s3.size()); + assertTrue(s3.containsAll(s2)); + + // delete a doc + assertU(delQ("foo2:doc")); + assertU(commit()); + + // same cores + assertEquals(s3, getCoreCacheKeys()); + } + + static void assertNRT(int maxDoc) { RefCounted searcher = h.getCore().getSearcher(); try { DirectoryReader ir = searcher.get().getIndexReader(); - assertEquals(1, ir.maxDoc()); + assertEquals(maxDoc, ir.maxDoc()); assertTrue("expected NRT reader, got: " + ir, ir.toString().contains(":nrt")); } finally { searcher.decref(); } } + + private Set getCoreCacheKeys() { + RefCounted searcher = h.getCore().getSearcher(); + Set set = Collections.newSetFromMap(new IdentityHashMap()); + try { + DirectoryReader ir = searcher.get().getIndexReader(); + for (AtomicReaderContext context : ir.leaves()) { + set.add(context.reader().getCoreCacheKey()); + } + } finally { + searcher.decref(); + } + return set; + } } diff --git a/solr/core/src/test/org/apache/solr/core/TestNonNRTOpen.java b/solr/core/src/test/org/apache/solr/core/TestNonNRTOpen.java new file mode 100644 index 00000000000..8a5e493a8cc --- /dev/null +++ b/solr/core/src/test/org/apache/solr/core/TestNonNRTOpen.java @@ -0,0 +1,154 @@ +package org.apache.solr.core; + +/* + * 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.io.File; +import java.util.Collections; +import java.util.IdentityHashMap; +import java.util.Set; + +import org.apache.lucene.index.AtomicReaderContext; +import org.apache.lucene.index.DirectoryReader; +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.search.SolrIndexSearcher; +import org.apache.solr.util.RefCounted; +import org.junit.AfterClass; +import org.junit.BeforeClass; + +public class TestNonNRTOpen extends SolrTestCaseJ4 { + + @BeforeClass + public static void beforeClass() throws Exception { + // use a filesystem, because we need to create an index, then "start up solr" + System.setProperty("solr.directoryFactory", "solr.StandardDirectoryFactory"); + // and dont delete it initially + System.setProperty("solr.test.leavedatadir", "true"); + // turn off nrt + System.setProperty("solr.tests.nrtMode", "false"); + // set these so that merges won't break the test + System.setProperty("solr.tests.maxBufferedDocs", "100000"); + System.setProperty("solr.tests.mergePolicy", "org.apache.lucene.index.LogDocMergePolicy"); + initCore("solrconfig-basic.xml", "schema-minimal.xml"); + // add a doc + assertU(adoc("foo", "bar")); + assertU(commit()); + File myDir = dataDir; + deleteCore(); + // boot up again over the same index + dataDir = myDir; + initCore("solrconfig-basic.xml", "schema-minimal.xml"); + // startup + assertNotNRT(1); + } + + public void setUp() throws Exception { + super.setUp(); + // delete all, then add initial doc + assertU(delQ("*:*")); + assertU(adoc("foo", "bar")); + assertU(commit()); + } + + @AfterClass + public static void afterClass() throws Exception { + // ensure we clean up after ourselves, this will fire before superclass... + System.clearProperty("solr.test.leavedatadir"); + System.clearProperty("solr.directoryFactory"); + System.clearProperty("solr.tests.maxBufferedDocs"); + System.clearProperty("solr.tests.mergePolicy"); + System.clearProperty("solr.tests.nrtMode"); + } + + public void testReaderIsNotNRT() { + // startup + assertNotNRT(1); + + // core reload + String core = h.getCore().getName(); + h.getCoreContainer().reload(core); + assertNotNRT(1); + + // add a doc and commit + assertU(adoc("baz", "doc")); + assertU(commit()); + assertNotNRT(2); + + // add a doc and core reload + assertU(adoc("bazz", "doc2")); + h.getCoreContainer().reload(core); + assertNotNRT(3); + } + + public void testSharedCores() { + // clear out any junk + assertU(optimize()); + + Set s1 = getCoreCacheKeys(); + assertEquals(1, s1.size()); + + // add a doc, will go in a new segment + assertU(adoc("baz", "doc")); + assertU(commit()); + + Set s2 = getCoreCacheKeys(); + assertEquals(2, s2.size()); + assertTrue(s2.containsAll(s1)); + + // add two docs, will go in a new segment + assertU(adoc("foo", "doc")); + assertU(adoc("foo2", "doc")); + assertU(commit()); + + Set s3 = getCoreCacheKeys(); + assertEquals(3, s3.size()); + assertTrue(s3.containsAll(s2)); + + // delete a doc + assertU(delQ("foo2:doc")); + assertU(commit()); + + // same cores + assertEquals(s3, getCoreCacheKeys()); + } + + static void assertNotNRT(int maxDoc) { + RefCounted searcher = h.getCore().getSearcher(); + try { + DirectoryReader ir = searcher.get().getIndexReader(); + assertEquals(maxDoc, ir.maxDoc()); + assertFalse("expected non-NRT reader, got: " + ir, ir.toString().contains(":nrt")); + } finally { + searcher.decref(); + } + } + + private Set getCoreCacheKeys() { + RefCounted searcher = h.getCore().getSearcher(); + Set set = Collections.newSetFromMap(new IdentityHashMap()); + try { + DirectoryReader ir = searcher.get().getIndexReader(); + for (AtomicReaderContext context : ir.leaves()) { + set.add(context.reader().getCoreCacheKey()); + } + } finally { + searcher.decref(); + } + return set; + } +} + diff --git a/solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java b/solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java index a0a05a06067..72921816b06 100644 --- a/solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java +++ b/solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java @@ -348,7 +348,7 @@ public class TestReplicationHandler extends SolrTestCaseJ4 { useFactory(null); // force a persistent directory // read-only setting (no opening from indexwriter) - System.setProperty("solr.tests.reopenReaders", "false"); + System.setProperty("solr.tests.nrtMode", "false"); try { // stop and start so they see the new directory setting slaveJetty.stop(); @@ -361,7 +361,7 @@ public class TestReplicationHandler extends SolrTestCaseJ4 { slaveJetty.stop(); slaveJetty.start(true); } finally { - System.clearProperty("solr.tests.reopenReaders"); // dont mess with other tests + System.clearProperty("solr.tests.nrtMode"); // dont mess with other tests } // Currently we open a writer on-demand. This is to test that we are correctly testing diff --git a/solr/example/example-schemaless/solr/collection1/conf/solrconfig.xml b/solr/example/example-schemaless/solr/collection1/conf/solrconfig.xml index 5ee229bb71e..f215426fb15 100755 --- a/solr/example/example-schemaless/solr/collection1/conf/solrconfig.xml +++ b/solr/example/example-schemaless/solr/collection1/conf/solrconfig.xml @@ -257,11 +257,13 @@ false --> - -