From 5b3cdaca447a44842f01eaa7c535daa4cd4ac5f0 Mon Sep 17 00:00:00 2001 From: Mark Robert Miller Date: Thu, 21 Mar 2013 22:44:14 +0000 Subject: [PATCH] SOLR-4624: CachingDirectoryFactory does not need to support forceNew any longer and it appears to be causing a missing close directory bug. forceNew is no longer respected and will be removed in 4.3. git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1459565 13f79535-47bb-0310-9956-ffa450edef68 --- solr/CHANGES.txt | 5 +++ .../solr/core/CachingDirectoryFactory.java | 45 +++---------------- .../apache/solr/core/DirectoryFactory.java | 15 ------- .../java/org/apache/solr/core/SolrCore.java | 2 +- .../solr/update/DefaultSolrCoreState.java | 8 ++-- .../apache/solr/update/SolrIndexSplitter.java | 2 +- .../apache/solr/update/SolrIndexWriter.java | 4 +- .../core/CachingDirectoryFactoryTest.java | 24 +--------- 8 files changed, 19 insertions(+), 86 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index c48227a4e15..8ddd2207193 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -96,6 +96,8 @@ Other Changes * SOLR-4607: Use noggit 0.5 release jar rather than a forked copy. (Yonik Seeley, Robert Muir) +* SOLR-4624: forceNew has been removed from the DirectoryFactory and related java apis. + ================== 4.2.1 ================== Versions of Major Components @@ -213,6 +215,9 @@ Bug Fixes values and top-level phrase slops on queries produced by nested sub-parsers. (yonik) +* SOLR-4624: CachingDirectoryFactory does not need to support forceNew any + longer and it appears to be causing a missing close directory bug. forceNew + is no longer respected and will be removed in 4.3. (Mark Miller) Optimizations ---------------------- 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 53710182fb8..2648c182a56 100644 --- a/solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java +++ b/solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java @@ -63,10 +63,6 @@ public abstract class CachingDirectoryFactory extends DirectoryFactory { this.closeEntries.add(this); } public int refCnt = 1; - // if we are latestForPath, I'm currently using my path - // otherwise a new Directory instance is using my path - // and I must be manipulated by Directory - public boolean latestForPath = false; // has close(Directory) been called on this? public boolean closeDirectoryCalled = false; public boolean doneWithDir = false; @@ -218,12 +214,8 @@ public abstract class CachingDirectoryFactory extends DirectoryFactory { byDirectoryCache.remove(directory); - // if it's been closed, it's path is now - // owned by another Directory instance - if (!cacheValue.latestForPath) { - byPathCache.remove(cacheValue.path); - cacheValue.latestForPath = true; - } + byPathCache.remove(cacheValue.path); + } } } @@ -310,18 +302,6 @@ public abstract class CachingDirectoryFactory extends DirectoryFactory { return dirFile.canRead() && dirFile.list().length > 0; } - /* - * (non-Javadoc) - * - * @see org.apache.solr.core.DirectoryFactory#get(java.lang.String, - * java.lang.String) - */ - @Override - public final Directory get(String path, DirContext dirContext, String rawLockType) - throws IOException { - return get(path, dirContext, rawLockType, false); - } - /* * (non-Javadoc) * @@ -329,7 +309,7 @@ public abstract class CachingDirectoryFactory extends DirectoryFactory { * java.lang.String, boolean) */ @Override - public final Directory get(String path, DirContext dirContext, String rawLockType, boolean forceNew) + public final Directory get(String path, DirContext dirContext, String rawLockType) throws IOException { String fullPath = normalize(path); synchronized (this) { @@ -341,24 +321,9 @@ public abstract class CachingDirectoryFactory extends DirectoryFactory { Directory directory = null; if (cacheValue != null) { directory = cacheValue.directory; - if (forceNew) { - cacheValue.doneWithDir = true; - - // we make a quick close attempt, - // otherwise this should be closed - // when whatever is using it, releases it - if (cacheValue.refCnt == 0) { - closeDirectory(cacheValue); - } - - // close the entry, it will be owned by the new dir - // we count on it being released by directory - cacheValue.latestForPath = true; - - } } - if (directory == null || forceNew) { + if (directory == null) { directory = create(fullPath, dirContext); directory = rateLimit(directory); @@ -369,7 +334,7 @@ public abstract class CachingDirectoryFactory extends DirectoryFactory { byDirectoryCache.put(directory, newCacheValue); byPathCache.put(fullPath, newCacheValue); - log.info("return new directory for " + fullPath + " forceNew: " + forceNew); + log.info("return new directory for " + fullPath); } else { cacheValue.refCnt++; } diff --git a/solr/core/src/java/org/apache/solr/core/DirectoryFactory.java b/solr/core/src/java/org/apache/solr/core/DirectoryFactory.java index 65484093295..a5b329c3d7a 100644 --- a/solr/core/src/java/org/apache/solr/core/DirectoryFactory.java +++ b/solr/core/src/java/org/apache/solr/core/DirectoryFactory.java @@ -140,27 +140,12 @@ public abstract class DirectoryFactory implements NamedListInitializedPlugin, * Returns the Directory for a given path, using the specified rawLockType. * Will return the same Directory instance for the same path. * - * Note: sometimes you might pass null for the rawLockType when - * you know the Directory exists and the rawLockType is already - * in use. * * @throws IOException If there is a low-level I/O error. */ public abstract Directory get(String path, DirContext dirContext, String rawLockType) throws IOException; - /** - * Returns the Directory for a given path, using the specified rawLockType. - * Will return the same Directory instance for the same path unless forceNew, - * in which case a new Directory is returned. There is no need to call - * {@link #doneWithDirectory(Directory)} in this case - the old Directory - * will be closed when it's ref count hits 0. - * - * @throws IOException If there is a low-level I/O error. - */ - public abstract Directory get(String path, DirContext dirContext, String rawLockType, - boolean forceNew) throws IOException; - /** * Increment the number of references to the given Directory. You must call * release for every call to this method. 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 900d72275c9..2a33a904dc1 100644 --- a/solr/core/src/java/org/apache/solr/core/SolrCore.java +++ b/solr/core/src/java/org/apache/solr/core/SolrCore.java @@ -497,7 +497,7 @@ public final class SolrCore implements SolrInfoMBean { log.warn(logid+"Solr index directory '" + new File(indexDir) + "' doesn't exist." + " Creating new index..."); - SolrIndexWriter writer = SolrIndexWriter.create("SolrCore.initIndex", indexDir, getDirectoryFactory(), true, schema, solrConfig.indexConfig, solrDelPolicy, codec, false); + SolrIndexWriter writer = SolrIndexWriter.create("SolrCore.initIndex", indexDir, getDirectoryFactory(), true, schema, solrConfig.indexConfig, solrDelPolicy, codec); writer.close(); } 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 432ebcec7d1..bcf33b64f03 100644 --- a/solr/core/src/java/org/apache/solr/update/DefaultSolrCoreState.java +++ b/solr/core/src/java/org/apache/solr/update/DefaultSolrCoreState.java @@ -104,7 +104,7 @@ public final class DefaultSolrCoreState extends SolrCoreState implements Recover } if (indexWriter == null) { - indexWriter = createMainIndexWriter(core, "DirectUpdateHandler2", false); + indexWriter = createMainIndexWriter(core, "DirectUpdateHandler2"); } initRefCntWriter(); writerFree = false; @@ -172,7 +172,7 @@ public final class DefaultSolrCoreState extends SolrCoreState implements Recover } } } - indexWriter = createMainIndexWriter(core, "DirectUpdateHandler2", forceNewDir); + indexWriter = createMainIndexWriter(core, "DirectUpdateHandler2"); 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; @@ -189,10 +189,10 @@ public final class DefaultSolrCoreState extends SolrCoreState implements Recover newIndexWriter(core, true, false); } - protected SolrIndexWriter createMainIndexWriter(SolrCore core, String name, boolean forceNewDirectory) throws IOException { + protected SolrIndexWriter createMainIndexWriter(SolrCore core, String name) throws IOException { return SolrIndexWriter.create(name, core.getNewIndexDir(), core.getDirectoryFactory(), false, core.getSchema(), - core.getSolrConfig().indexConfig, core.getDeletionPolicy(), core.getCodec(), forceNewDirectory); + core.getSolrConfig().indexConfig, core.getDeletionPolicy(), core.getCodec()); } @Override diff --git a/solr/core/src/java/org/apache/solr/update/SolrIndexSplitter.java b/solr/core/src/java/org/apache/solr/update/SolrIndexSplitter.java index fb87d940baf..04f5f730eaf 100644 --- a/solr/core/src/java/org/apache/solr/update/SolrIndexSplitter.java +++ b/solr/core/src/java/org/apache/solr/update/SolrIndexSplitter.java @@ -102,7 +102,7 @@ public class SolrIndexSplitter { String path = paths.get(partitionNumber); iw = SolrIndexWriter.create("SplittingIndexWriter"+partitionNumber + " " + ranges.get(partitionNumber), path, core.getDirectoryFactory(), true, core.getSchema(), - core.getSolrConfig().indexConfig, core.getDeletionPolicy(), core.getCodec(), true); + core.getSolrConfig().indexConfig, core.getDeletionPolicy(), core.getCodec()); } try { 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 cbfc880067c..fb23a1f3e7b 100644 --- a/solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java +++ b/solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java @@ -56,10 +56,10 @@ public class SolrIndexWriter extends IndexWriter { String name; private DirectoryFactory directoryFactory; - public static SolrIndexWriter create(String name, String path, DirectoryFactory directoryFactory, boolean create, IndexSchema schema, SolrIndexConfig config, IndexDeletionPolicy delPolicy, Codec codec, boolean forceNewDirectory) throws IOException { + public static SolrIndexWriter create(String name, String path, DirectoryFactory directoryFactory, boolean create, IndexSchema schema, SolrIndexConfig config, IndexDeletionPolicy delPolicy, Codec codec) throws IOException { SolrIndexWriter w = null; - final Directory d = directoryFactory.get(path, DirContext.DEFAULT, config.lockType, forceNewDirectory); + final Directory d = directoryFactory.get(path, DirContext.DEFAULT, config.lockType); try { w = new SolrIndexWriter(name, path, d, create, schema, config, delPolicy, codec); diff --git a/solr/core/src/test/org/apache/solr/core/CachingDirectoryFactoryTest.java b/solr/core/src/test/org/apache/solr/core/CachingDirectoryFactoryTest.java index 3e0eff2e790..c5cf655d279 100644 --- a/solr/core/src/test/org/apache/solr/core/CachingDirectoryFactoryTest.java +++ b/solr/core/src/test/org/apache/solr/core/CachingDirectoryFactoryTest.java @@ -32,7 +32,6 @@ import org.junit.Test; public class CachingDirectoryFactoryTest extends SolrTestCaseJ4 { private Map dirs = new HashMap(); - private List oldDirs = new ArrayList(); private volatile boolean stop = false; private class Tracker { @@ -96,16 +95,6 @@ public class CachingDirectoryFactoryTest extends SolrTestCaseJ4 { } } } - sz = oldDirs.size(); - if (sz > 0) { - for (Tracker tracker : oldDirs) { - int cnt = tracker.refCnt.get(); - for (int i = 0; i < cnt; i++) { - tracker.refCnt.decrementAndGet(); - df.release(tracker.dir); - } - } - } } @@ -187,18 +176,7 @@ public class CachingDirectoryFactoryTest extends SolrTestCaseJ4 { tracker.dir = df.get(path, DirContext.DEFAULT, null); dirs.put(path, tracker); } else { - if (random.nextInt(10) > 6) { - Tracker oldTracker = new Tracker(); - oldTracker.refCnt = new AtomicInteger(tracker.refCnt.get()); - oldTracker.path = tracker.path; - oldTracker.dir = tracker.dir; - oldDirs.add(oldTracker); - - tracker.dir = df.get(path, DirContext.DEFAULT, null, true); - tracker.refCnt = new AtomicInteger(0); - } else { - tracker.dir = df.get(path, DirContext.DEFAULT, null); - } + tracker.dir = df.get(path, DirContext.DEFAULT, null); } tracker.refCnt.incrementAndGet(); }