From 12cbfaf62a17c12697262b7f74aed9656d95e8d1 Mon Sep 17 00:00:00 2001 From: Mark Robert Miller Date: Wed, 15 Aug 2012 13:34:34 +0000 Subject: [PATCH] SOLR-3730: Rollback is not implemented quite right and can cause corner case fails in SolrCloud tests. git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1373398 13f79535-47bb-0310-9956-ffa450edef68 --- solr/CHANGES.txt | 3 ++ .../solr/core/CachingDirectoryFactory.java | 2 ++ .../java/org/apache/solr/core/SolrCore.java | 2 +- .../org/apache/solr/handler/SnapPuller.java | 6 ++-- .../solr/update/DefaultSolrCoreState.java | 35 +++++++++++-------- .../apache/solr/update/SolrIndexWriter.java | 2 ++ .../solr/collection1/conf/solrconfig.xml | 2 +- .../solr/core/MockDirectoryFactory.java | 10 +++--- .../solr/core/MockFSDirectoryFactory.java | 10 +++--- 9 files changed, 45 insertions(+), 27 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 71a59093e23..de08c68cf11 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -63,6 +63,9 @@ Bug Fixes * SOLR-3649: Fixed bug in JavabinLoader that caused deleteById(List ids) to not work in SolrJ (siren) +* SOLR-3730: Rollback is not implemented quite right and can cause corner case fails in + SolrCloud tests. (rmuir, Mark Miller) + ================== 4.0.0-BETA =================== diff --git a/solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java b/solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java index e98fd5fcfb2..c5c7c4e1b36 100644 --- a/solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java +++ b/solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java @@ -125,6 +125,7 @@ public abstract class CachingDirectoryFactory extends DirectoryFactory { } cacheValue.refCnt--; if (cacheValue.refCnt == 0 && cacheValue.doneWithDir) { + log.info("Closing directory:" + cacheValue.path); directory.close(); byDirectoryCache.remove(directory); byPathCache.remove(cacheValue.path); @@ -194,6 +195,7 @@ public abstract class CachingDirectoryFactory extends DirectoryFactory { byDirectoryCache.put(directory, newCacheValue); byPathCache.put(fullPath, newCacheValue); + log.info("return new directory for " + fullPath + " forceNew:" + forceNew); } else { cacheValue.refCnt++; } 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 b58546fd6c3..670972f8879 100644 --- a/solr/core/src/java/org/apache/solr/core/SolrCore.java +++ b/solr/core/src/java/org/apache/solr/core/SolrCore.java @@ -1554,7 +1554,7 @@ public final class SolrCore implements SolrInfoMBean { } catch (Throwable e) { // do not allow decref() operations to fail since they are typically called in finally blocks // and throwing another exception would be very unexpected. - SolrException.log(log, "Error closing searcher:", e); + SolrException.log(log, "Error closing searcher:" + this, e); } } }; diff --git a/solr/core/src/java/org/apache/solr/handler/SnapPuller.java b/solr/core/src/java/org/apache/solr/handler/SnapPuller.java index 72d52263f0a..6b9291fcecd 100644 --- a/solr/core/src/java/org/apache/solr/handler/SnapPuller.java +++ b/solr/core/src/java/org/apache/solr/handler/SnapPuller.java @@ -384,7 +384,7 @@ public class SnapPuller { // may be closed core.getDirectoryFactory().doneWithDirectory(oldDirectory); } - doCommit(); + doCommit(isFullCopyNeeded); } replicationStartTime = 0; @@ -533,11 +533,11 @@ public class SnapPuller { return sb; } - private void doCommit() throws IOException { + private void doCommit(boolean isFullCopyNeeded) throws IOException { SolrQueryRequest req = new LocalSolrQueryRequest(solrCore, new ModifiableSolrParams()); // reboot the writer on the new index and get a new searcher - solrCore.getUpdateHandler().newIndexWriter(true); + solrCore.getUpdateHandler().newIndexWriter(isFullCopyNeeded); try { // first try to open an NRT searcher so that the new diff --git a/solr/core/src/java/org/apache/solr/update/DefaultSolrCoreState.java b/solr/core/src/java/org/apache/solr/update/DefaultSolrCoreState.java index 67b43448ed3..6a53ed5e763 100644 --- a/solr/core/src/java/org/apache/solr/update/DefaultSolrCoreState.java +++ b/solr/core/src/java/org/apache/solr/update/DefaultSolrCoreState.java @@ -74,8 +74,7 @@ public final class DefaultSolrCoreState extends SolrCoreState { } if (indexWriter == null) { - indexWriter = createMainIndexWriter(core, "DirectUpdateHandler2", - false, false); + indexWriter = createMainIndexWriter(core, "DirectUpdateHandler2", false); } if (refCntWriter == null) { refCntWriter = new RefCounted(indexWriter) { @@ -110,18 +109,28 @@ public final class DefaultSolrCoreState extends SolrCoreState { writerPauseLock.wait(); } catch (InterruptedException e) {} } - + try { if (indexWriter != null) { - try { - log.info("Closing old IndexWriter... core=" + coreName); - indexWriter.close(); - } catch (Throwable t) { - SolrException.log(log, "Error closing old IndexWriter. core=" + coreName, t); + if (!rollback) { + try { + log.info("Closing old IndexWriter... core=" + coreName); + indexWriter.close(); + } catch (Throwable t) { + SolrException.log(log, "Error closing old IndexWriter. core=" + + coreName, t); + } + } else { + try { + log.info("Rollback old IndexWriter... core=" + coreName); + indexWriter.rollback(); + } catch (Throwable t) { + SolrException.log(log, "Error rolling back old IndexWriter. core=" + + coreName, t); + } } } - indexWriter = createMainIndexWriter(core, "DirectUpdateHandler2", - false, true); + indexWriter = createMainIndexWriter(core, "DirectUpdateHandler2", true); log.info("New IndexWriter is ready to be used."); // we need to null this so it picks up the new writer next get call refCntWriter = null; @@ -174,14 +183,12 @@ public final class DefaultSolrCoreState extends SolrCoreState { @Override public synchronized void rollbackIndexWriter(SolrCore core) throws IOException { - indexWriter.rollback(); newIndexWriter(core, true); } - protected SolrIndexWriter createMainIndexWriter(SolrCore core, String name, - boolean removeAllExisting, boolean forceNewDirectory) throws IOException { + protected SolrIndexWriter createMainIndexWriter(SolrCore core, String name, boolean forceNewDirectory) throws IOException { return new SolrIndexWriter(name, core.getNewIndexDir(), - core.getDirectoryFactory(), removeAllExisting, core.getSchema(), + core.getDirectoryFactory(), false, core.getSchema(), core.getSolrConfig().indexConfig, core.getDeletionPolicy(), core.getCodec(), forceNewDirectory); } diff --git a/solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java b/solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java index d59164a9344..56dbca1992a 100644 --- a/solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java +++ b/solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java @@ -141,6 +141,8 @@ public class SolrIndexWriter extends IndexWriter { super.rollback(); } finally { isClosed = true; + directoryFactory.release(getDirectory()); + numCloses.incrementAndGet(); } } 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 7a7ba7d547a..9b00044669c 100644 --- a/solr/core/src/test-files/solr/collection1/conf/solrconfig.xml +++ b/solr/core/src/test-files/solr/collection1/conf/solrconfig.xml @@ -54,7 +54,7 @@ --> 10 - single + native true diff --git a/solr/test-framework/src/java/org/apache/solr/core/MockDirectoryFactory.java b/solr/test-framework/src/java/org/apache/solr/core/MockDirectoryFactory.java index d0061a7c91b..4c55dd376be 100644 --- a/solr/test-framework/src/java/org/apache/solr/core/MockDirectoryFactory.java +++ b/solr/test-framework/src/java/org/apache/solr/core/MockDirectoryFactory.java @@ -32,10 +32,12 @@ public class MockDirectoryFactory extends CachingDirectoryFactory { @Override protected Directory create(String path) throws IOException { Directory dir = LuceneTestCase.newDirectory(); - // Somehow removing unref'd files in Solr tests causes - // problems... there's some interaction w/ - // CachingDirectoryFactory. Once we track down where Solr - // isn't closing an IW, we can re-enable this: + // we can't currently do this check because of how + // Solr has to reboot a new Directory sometimes when replicating + // or rolling back - the old directory is closed and the following + // test assumes it can open an IndexWriter when that happens - we + // have a new Directory for the same dir and still an open IW at + // this point if (dir instanceof MockDirectoryWrapper) { ((MockDirectoryWrapper)dir).setAssertNoUnrefencedFilesOnClose(false); } diff --git a/solr/test-framework/src/java/org/apache/solr/core/MockFSDirectoryFactory.java b/solr/test-framework/src/java/org/apache/solr/core/MockFSDirectoryFactory.java index c83e602f9e9..4a23fbc5d5f 100644 --- a/solr/test-framework/src/java/org/apache/solr/core/MockFSDirectoryFactory.java +++ b/solr/test-framework/src/java/org/apache/solr/core/MockFSDirectoryFactory.java @@ -32,10 +32,12 @@ public class MockFSDirectoryFactory extends CachingDirectoryFactory { @Override public Directory create(String path) throws IOException { Directory dir = LuceneTestCase.newFSDirectory(new File(path)); - // Somehow removing unref'd files in Solr tests causes - // problems... there's some interaction w/ - // CachingDirectoryFactory. Once we track down where Solr - // isn't closing an IW, we can re-enable this: + // we can't currently do this check because of how + // Solr has to reboot a new Directory sometimes when replicating + // or rolling back - the old directory is closed and the following + // test assumes it can open an IndexWriter when that happens - we + // have a new Directory for the same dir and still an open IW at + // this point if (dir instanceof MockDirectoryWrapper) { ((MockDirectoryWrapper)dir).setAssertNoUnrefencedFilesOnClose(false); }