diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 05574f64371..80e4820ff25 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -167,6 +167,8 @@ Improvements * SOLR-17390: EmbeddedSolrServer now considers the ResponseParser (David Smiley) +* SOLR-17504: CoreContainer calls UpdateHandler.commit when closing a read-only core (Bruno Roustant) + Optimizations --------------------- * SOLR-14985: Solrj CloudSolrClient with Solr URLs had serious performance regressions (since the diff --git a/solr/core/src/java/org/apache/solr/core/CoreContainer.java b/solr/core/src/java/org/apache/solr/core/CoreContainer.java index a92c73ef496..69b6f07fb47 100644 --- a/solr/core/src/java/org/apache/solr/core/CoreContainer.java +++ b/solr/core/src/java/org/apache/solr/core/CoreContainer.java @@ -59,7 +59,6 @@ import org.apache.http.client.CredentialsProvider; import org.apache.http.config.Lookup; import org.apache.lucene.index.CorruptIndexException; -import org.apache.lucene.index.IndexWriter; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.store.Directory; import org.apache.lucene.util.BytesRef; @@ -95,6 +94,7 @@ import org.apache.solr.common.cloud.Replica.State; import org.apache.solr.common.cloud.SolrZkClient; import org.apache.solr.common.cloud.ZkStateReader; +import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.util.CollectionUtil; import org.apache.solr.common.util.ExecutorUtil; import org.apache.solr.common.util.IOUtils; @@ -138,6 +138,8 @@ import org.apache.solr.metrics.SolrMetricProducer; import org.apache.solr.metrics.SolrMetricsContext; import org.apache.solr.pkg.SolrPackageLoader; +import org.apache.solr.request.LocalSolrQueryRequest; +import org.apache.solr.request.SolrQueryRequest; import org.apache.solr.request.SolrRequestHandler; import org.apache.solr.request.SolrRequestInfo; import org.apache.solr.search.CacheConfig; @@ -153,10 +155,10 @@ import org.apache.solr.security.PublicKeyHandler; import org.apache.solr.security.SecurityPluginHolder; import org.apache.solr.security.SolrNodeKeyPair; +import org.apache.solr.update.CommitUpdateCommand; import org.apache.solr.update.SolrCoreState; import org.apache.solr.update.UpdateShardHandler; import org.apache.solr.util.OrderedExecutor; -import org.apache.solr.util.RefCounted; import org.apache.solr.util.StartupLoggingUtils; import org.apache.solr.util.stats.MetricUtils; import org.apache.zookeeper.KeeperException; @@ -2058,19 +2060,8 @@ public void reload(String name, UUID coreId) { // force commit on old core if the new one is readOnly and prevent any new updates if (newCore.readOnly) { - RefCounted iwRef = core.getSolrCoreState().getIndexWriter(null); - if (iwRef != null) { - IndexWriter iw = iwRef.get(); - // switch old core to readOnly - core.readOnly = true; - try { - if (iw != null) { - iw.commit(); - } - } finally { - iwRef.decref(); - } - } + SolrQueryRequest req = new LocalSolrQueryRequest(core, new ModifiableSolrParams()); + core.getUpdateHandler().commit(CommitUpdateCommand.closeOnCommit(req, false)); } if (docCollection != null) { diff --git a/solr/core/src/java/org/apache/solr/update/CommitUpdateCommand.java b/solr/core/src/java/org/apache/solr/update/CommitUpdateCommand.java index bb18b770e5d..6a749af2ac9 100644 --- a/solr/core/src/java/org/apache/solr/update/CommitUpdateCommand.java +++ b/solr/core/src/java/org/apache/solr/update/CommitUpdateCommand.java @@ -21,9 +21,21 @@ /** A commit index command encapsulated in an object. */ public class CommitUpdateCommand extends UpdateCommand { + + // Behavior about opening a searcher after a hard commit. + // - Open a standard searcher and wait for it. (default) + // openSearcher == true && waitSearcher == true && closeSearcher == false + // - Open a standard searcher, but do not wait for it. + // openSearcher == true && waitSearcher == false && closeSearcher == false + // - Open a real-time searcher quicker without auto-warming. + // openSearcher == false && closeSearcher == false + // - Do not open any searcher and prevent further updates (e.g. when the core is closing). + // closeSearcher == true + public boolean optimize; public boolean openSearcher = true; // open a new searcher as part of a hard commit public boolean waitSearcher = true; + private boolean closeSearcher = false; public boolean expungeDeletes = false; public boolean softCommit = false; public boolean prepareCommit = false; @@ -47,6 +59,26 @@ public CommitUpdateCommand(SolrQueryRequest req, boolean optimize) { this.optimize = optimize; } + /** + * Creates a {@link CommitUpdateCommand} to commit before closing the core and also prevent any + * new update by setting the core in read-only mode. Does not open a new searcher. + */ + public static CommitUpdateCommand closeOnCommit(SolrQueryRequest req, boolean optimize) { + CommitUpdateCommand cmd = new CommitUpdateCommand(req, optimize); + cmd.openSearcher = false; + cmd.waitSearcher = false; + cmd.closeSearcher = true; + return cmd; + } + + /** + * Indicates whether this command is a commit before the core is closed. Any new updates must be + * prevented after the commit. + */ + public boolean isClosingOnCommit() { + return closeSearcher; + } + @Override public String name() { return "commit"; diff --git a/solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java b/solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java index d730cc137fb..2704ef4e027 100644 --- a/solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java +++ b/solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java @@ -738,6 +738,9 @@ public void commit(CommitUpdateCommand cmd) throws IOException { RefCounted iw = solrCoreState.getIndexWriter(core); try { + if (cmd.isClosingOnCommit()) { + core.readOnly = true; + } IndexWriter writer = iw.get(); if (cmd.optimize) { writer.forceMerge(cmd.maxOptimizeSegments); @@ -786,7 +789,7 @@ public void commit(CommitUpdateCommand cmd) throws IOException { if (ulog != null) ulog.preSoftCommit(cmd); if (cmd.openSearcher) { core.getSearcher(true, false, waitSearcher); - } else { + } else if (!cmd.isClosingOnCommit()) { // force open a new realtime searcher so realtime-get and versioning code can see the // latest RefCounted searchHolder = core.openNewSearcher(true, true); @@ -971,8 +974,10 @@ public void closeWriter(IndexWriter writer) throws IOException { // todo: refactor this shared code (or figure out why a real CommitUpdateCommand can't // be used) - SolrIndexWriter.setCommitData(writer, cmd.getVersion(), null); - writer.commit(); + if (shouldCommit(cmd, writer)) { + SolrIndexWriter.setCommitData(writer, cmd.getVersion(), cmd.commitData); + writer.commit(); + } synchronized (solrCoreState.getUpdateLock()) { ulog.postCommit(cmd); diff --git a/solr/core/src/test-files/solr/collection1/conf/solrconfig.xml b/solr/core/src/test-files/solr/collection1/conf/solrconfig.xml index 2c06096732c..856079f329e 100644 --- a/solr/core/src/test-files/solr/collection1/conf/solrconfig.xml +++ b/solr/core/src/test-files/solr/collection1/conf/solrconfig.xml @@ -59,7 +59,7 @@ - + ${solr.autoCommit.maxTime:-1} diff --git a/solr/core/src/test/org/apache/solr/update/DirectUpdateHandlerWithUpdateLogTest.java b/solr/core/src/test/org/apache/solr/update/DirectUpdateHandlerWithUpdateLogTest.java new file mode 100644 index 00000000000..49a3be19697 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/update/DirectUpdateHandlerWithUpdateLogTest.java @@ -0,0 +1,98 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.update; + +import java.io.IOException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.lucene.index.IndexWriter; +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.util.TimeSource; +import org.apache.solr.core.SolrCore; +import org.apache.solr.util.LogLevel; +import org.apache.solr.util.TimeOut; +import org.junit.BeforeClass; +import org.junit.Test; + +/** Tests {@link DirectUpdateHandler2} with update log enabled. */ +@LogLevel("org.apache.solr.update=INFO") +public class DirectUpdateHandlerWithUpdateLogTest extends SolrTestCaseJ4 { + + @BeforeClass + public static void beforeClass() throws Exception { + System.setProperty("solr.updateHandler", SpyingUpdateHandler.class.getName()); + initCore("solrconfig.xml", "schema11.xml"); + } + + @Test + public void testShouldCommitHook() throws Exception { + // Given a core. + SolrCore core = h.getCore(); + assertNotNull(core); + SpyingUpdateHandler updater = (SpyingUpdateHandler) core.getUpdateHandler(); + updater.shouldCommitCallCount.set(0); + + // When we add a doc and commit. + assertU(adoc("id", "1")); + assertU(commit()); + // Then the shouldCommit hook is called. + assertEquals(1, updater.shouldCommitCallCount.get()); + + // When we add a doc and soft commit. + assertU(adoc("id", "2")); + assertU(commit("softCommit", "true")); + // Then the shouldCommit hook is not called. + assertEquals(1, updater.shouldCommitCallCount.get()); + // And when we commit. + assertU(commit()); + // Then the shouldCommit hook is called. + assertEquals(2, updater.shouldCommitCallCount.get()); + + // When we commit with no updates (empty commit). + assertU(commit()); + // Then the shouldCommit hook is called (may commit only user metadata). + assertEquals(3, updater.shouldCommitCallCount.get()); + + // When we add a doc, do not commit, and close the IndexWriter. + assertU(adoc("id", "3")); + h.close(); + // Then the shouldCommit hook is called. + new TimeOut(10, TimeUnit.SECONDS, TimeSource.NANO_TIME) + .waitFor( + "Timeout waiting for should commit hook", + () -> updater.shouldCommitCallCount.get() == 4); + } + + public static class SpyingUpdateHandler extends DirectUpdateHandler2 { + + final AtomicInteger shouldCommitCallCount = new AtomicInteger(); + + public SpyingUpdateHandler(SolrCore core) { + super(core); + } + + public SpyingUpdateHandler(SolrCore core, UpdateHandler updateHandler) { + super(core, updateHandler); + } + + @Override + protected boolean shouldCommit(CommitUpdateCommand cmd, IndexWriter writer) throws IOException { + shouldCommitCallCount.incrementAndGet(); + return super.shouldCommit(cmd, writer); + } + } +}