diff --git a/CHANGES.txt b/CHANGES.txt index 5d87f53a005..ca26daabfd9 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -425,6 +425,16 @@ API Changes 38. LUCENE-1847: Similarity#idf for both a Term and Term Collection have been deprecated. New versions that return an IDFExplanation have been added. (Yasoja Seneviratne, Mike McCandless, Mark Miller) + +39. LUCENE-1877: Made NativeFSLockFactory the default for + the new FSDirectory API (open(), FSDirectory subclass ctors). + All FSDirectory system properties were deprecated and all lock + implementations use no lock prefix if the locks are stored inside + the index directory. Because the deprecated String/File ctors of + IndexWriter and IndexReader (LUCENE-1672) and FSDirectory.getDirectory() + still use the old SimpleFSLockFactory and the new API + NativeFSLockFactory, we strongly recommend not to mix deprecated + and new API. (Uwe Schindler, Mike McCandless) Bug fixes @@ -534,6 +544,11 @@ Bug fixes in their Weight#explain methods - these stats should be corpus wide. (Yasoja Seneviratne, Mike McCandless, Mark Miller) +27. LUCENE-1885: Fix the bug that NativeFSLock.isLocked() did not work, + if the lock was obtained by another NativeFSLock(Factory) instance. + Because of this IndexReader.isLocked() and IndexWriter.isLocked() did + not work correctly. (Uwe Schindler) + New features 1. LUCENE-1411: Added expert API to open an IndexWriter on a prior diff --git a/common-build.xml b/common-build.xml index 9f14c1e2c04..64533e3dfbc 100644 --- a/common-build.xml +++ b/common-build.xml @@ -42,7 +42,7 @@ - + diff --git a/src/java/org/apache/lucene/store/FSDirectory.java b/src/java/org/apache/lucene/store/FSDirectory.java index 113b66d0f99..611d7afd0cd 100644 --- a/src/java/org/apache/lucene/store/FSDirectory.java +++ b/src/java/org/apache/lucene/store/FSDirectory.java @@ -95,12 +95,20 @@ import org.apache.lucene.index.IndexWriter; * desired implementation directly. * *

The locking implementation is by default {@link - * SimpleFSLockFactory}, but can be changed either by - * passing in a custom {@link LockFactory} instance, or - * specifying the LockFactory class by setting + * NativeFSLockFactory}, but can be changed by + * passing in a custom {@link LockFactory} instance. + * The deprecated getDirectory methods default to use + * {@link SimpleFSLockFactory} for backwards compatibility. + * The system properties * org.apache.lucene.store.FSDirectoryLockFactoryClass - * Java system property, or by calling {@link - * #setLockFactory} after creating the Directory. + * and org.apache.lucene.FSDirectory.class + * are deprecated and only used by the deprecated + * getDirectory methods. The system property + * org.apache.lucene.lockDir is ignored completely, + * If you really want to store locks + * elsewhere, you can create your own {@link + * SimpleFSLockFactory} (or {@link NativeFSLockFactory}, + * etc.) passing in your preferred lock directory. * *

In 3.0 this class will become abstract. * @@ -129,6 +137,11 @@ public class FSDirectory extends Directory { * Set whether Lucene's use of lock files is disabled. By default, * lock files are enabled. They should only be disabled if the index * is on a read-only medium like a CD-ROM. + * @deprecated Use a {@link #open(File, LockFactory)} or a constructor + * that takes a {@link LockFactory} and supply + * {@link NoLockFactory#getNoLockFactory}. This setting does not work + * with {@link #open(File)} only the deprecated getDirectory + * respect this setting. */ public static void setDisableLocks(boolean doDisableLocks) { FSDirectory.disableLocks = doDisableLocks; @@ -138,6 +151,8 @@ public class FSDirectory extends Directory { * Returns whether Lucene's use of lock files is disabled. * @return true if locks are disabled, false if locks are enabled. * @see #setDisableLocks + * @deprecated Use a constructor that takes a {@link LockFactory} and + * supply {@link NoLockFactory#getNoLockFactory}. */ public static boolean getDisableLocks() { return FSDirectory.disableLocks; @@ -150,12 +165,12 @@ public class FSDirectory extends Directory { * @deprecated As of 2.1, LOCK_DIR is unused * because the write.lock is now stored by default in the * index directory. If you really want to store locks - * elsewhere you can create your own {@link + * elsewhere, you can create your own {@link * SimpleFSLockFactory} (or {@link NativeFSLockFactory}, * etc.) passing in your preferred lock directory. Then, * pass this LockFactory instance to one of - * the getDirectory methods that take a - * lockFactory (for example, {@link #getDirectory(String, LockFactory)}). + * the open methods that take a + * lockFactory (for example, {@link #open(File, LockFactory)}). */ public static final String LOCK_DIR = System.getProperty("org.apache.lucene.lockDir", System.getProperty("java.io.tmpdir")); @@ -358,17 +373,23 @@ public class FSDirectory extends Directory { /** Create a new FSDirectory for the named location (ctor for subclasses). * @param path the path of the directory - * @param lockFactory the lock factory to use, or null for the default. + * @param lockFactory the lock factory to use, or null for the default + * ({@link NativeFSLockFactory}); * @throws IOException */ protected FSDirectory(File path, LockFactory lockFactory) throws IOException { path = getCanonicalPath(path); + // new ctors use always NativeFSLockFactory as default: + if (lockFactory == null) { + lockFactory = new NativeFSLockFactory(); + } init(path, lockFactory); refCount = 1; } /** Creates an FSDirectory instance, trying to pick the * best implementation given the current environment. + * The directory returned uses the {@link NativeFSLockFactory}. * *

Currently this returns {@link NIOFSDirectory} * on non-Windows JREs and {@link SimpleFSDirectory} @@ -419,8 +440,6 @@ public class FSDirectory extends Directory { if (directory.exists() && !directory.isDirectory()) throw new NoSuchDirectoryException("file '" + directory + "' exists but is not a directory"); - boolean doClearLockID = false; - if (lockFactory == null) { if (disableLocks) { @@ -447,27 +466,28 @@ public class FSDirectory extends Directory { } catch (ClassCastException e) { throw new IOException("unable to cast LockClass " + lockClassName + " instance to a LockFactory"); } - - if (lockFactory instanceof NativeFSLockFactory) { - ((NativeFSLockFactory) lockFactory).setLockDir(path); - } else if (lockFactory instanceof SimpleFSLockFactory) { - ((SimpleFSLockFactory) lockFactory).setLockDir(path); - } } else { // Our default lock is SimpleFSLockFactory; // default lockDir is our index directory: - lockFactory = new SimpleFSLockFactory(path); - doClearLockID = true; + lockFactory = new SimpleFSLockFactory(); } } } setLockFactory(lockFactory); - - if (doClearLockID) { - // Clear the prefix because write.lock will be - // stored in our directory: - lockFactory.setLockPrefix(null); + + // for filesystem based LockFactory, delete the lockPrefix, if the locks are placed + // in index dir. If no index dir is given, set ourselves + if (lockFactory instanceof FSLockFactory) { + final FSLockFactory lf = (FSLockFactory) lockFactory; + final File dir = lf.getLockDir(); + // if the lock factory has no lockDir set, use the this directory as lockDir + if (dir == null) { + lf.setLockDir(this.directory); + lf.setLockPrefix(null); + } else if (dir.getCanonicalPath().equals(this.directory.getCanonicalPath())) { + lf.setLockPrefix(null); + } } } diff --git a/src/java/org/apache/lucene/store/FSLockFactory.java b/src/java/org/apache/lucene/store/FSLockFactory.java new file mode 100644 index 00000000000..6d47582f334 --- /dev/null +++ b/src/java/org/apache/lucene/store/FSLockFactory.java @@ -0,0 +1,53 @@ +package org.apache.lucene.store; + +/** + * 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; + +/** + * Base class for file system based locking implementation. + */ + +public abstract class FSLockFactory extends LockFactory { + + /** + * Directory for the lock files. + */ + protected File lockDir = null; + + /** + * Set the lock directory. This method can be only called + * once to initialize the lock directory. It is used by {@link FSDirectory} + * to set the lock directory to itsself. + * Subclasses can also use this method to set the directory + * in the constructor. + */ + protected void setLockDir(File lockDir) { + if (this.lockDir != null) + throw new IllegalStateException("You can set the lock directory for this factory only once."); + this.lockDir = lockDir; + } + + /** + * Retrieve the lock directory. + */ + public File getLockDir() { + return lockDir; + } + +} diff --git a/src/java/org/apache/lucene/store/LockFactory.java b/src/java/org/apache/lucene/store/LockFactory.java index d1a515eb174..22ea6d4e8f0 100755 --- a/src/java/org/apache/lucene/store/LockFactory.java +++ b/src/java/org/apache/lucene/store/LockFactory.java @@ -35,7 +35,7 @@ import java.io.IOException; public abstract class LockFactory { - protected String lockPrefix = ""; + protected String lockPrefix = null; /** * Set the prefix in use for all locks created in this diff --git a/src/java/org/apache/lucene/store/MMapDirectory.java b/src/java/org/apache/lucene/store/MMapDirectory.java index af528609a93..13c8d52b986 100644 --- a/src/java/org/apache/lucene/store/MMapDirectory.java +++ b/src/java/org/apache/lucene/store/MMapDirectory.java @@ -74,14 +74,15 @@ public class MMapDirectory extends FSDirectory { /** Create a new MMapDirectory for the named location. * * @param path the path of the directory - * @param lockFactory the lock factory to use, or null for the default. + * @param lockFactory the lock factory to use, or null for the default + * ({@link NativeFSLockFactory}); * @throws IOException */ public MMapDirectory(File path, LockFactory lockFactory) throws IOException { super(path, lockFactory); } - /** Create a new MMapDirectory for the named location and the default lock factory. + /** Create a new MMapDirectory for the named location and {@link NativeFSLockFactory}. * * @param path the path of the directory * @throws IOException diff --git a/src/java/org/apache/lucene/store/NIOFSDirectory.java b/src/java/org/apache/lucene/store/NIOFSDirectory.java index 9cc754c5cdd..e1356b57c75 100644 --- a/src/java/org/apache/lucene/store/NIOFSDirectory.java +++ b/src/java/org/apache/lucene/store/NIOFSDirectory.java @@ -43,14 +43,15 @@ public class NIOFSDirectory extends FSDirectory { /** Create a new NIOFSDirectory for the named location. * * @param path the path of the directory - * @param lockFactory the lock factory to use, or null for the default. + * @param lockFactory the lock factory to use, or null for the default + * ({@link NativeFSLockFactory}); * @throws IOException */ public NIOFSDirectory(File path, LockFactory lockFactory) throws IOException { super(path, lockFactory); } - /** Create a new NIOFSDirectory for the named location and the default lock factory. + /** Create a new NIOFSDirectory for the named location and {@link NativeFSLockFactory}. * * @param path the path of the directory * @throws IOException diff --git a/src/java/org/apache/lucene/store/NativeFSLockFactory.java b/src/java/org/apache/lucene/store/NativeFSLockFactory.java index 665f1ba1166..ba6a070855e 100755 --- a/src/java/org/apache/lucene/store/NativeFSLockFactory.java +++ b/src/java/org/apache/lucene/store/NativeFSLockFactory.java @@ -55,45 +55,50 @@ import java.util.Random; * @see LockFactory */ -public class NativeFSLockFactory extends LockFactory { +public class NativeFSLockFactory extends FSLockFactory { - /** - * Directory specified by org.apache.lucene.lockDir - * system property. If that is not set, then java.io.tmpdir - * system property is used. - */ - - private File lockDir; + private volatile boolean tested = false; // Simple test to verify locking system is "working". On // NFS, if it's misconfigured, you can hit long (35 // second) timeouts which cause Lock.obtain to take far // too long (it assumes the obtain() call takes zero - // time). Since it's a configuration problem, we test up - // front once on creating the LockFactory: - private void acquireTestLock() throws IOException { + // time). + private synchronized void acquireTestLock() { + if (tested) return; + tested = true; + + // Ensure that lockDir exists and is a directory. + if (!lockDir.exists()) { + if (!lockDir.mkdirs()) + throw new RuntimeException("Cannot create directory: " + + lockDir.getAbsolutePath()); + } else if (!lockDir.isDirectory()) { + throw new RuntimeException("Found regular file where directory expected: " + + lockDir.getAbsolutePath()); + } + String randomLockName = "lucene-" + Long.toString(new Random().nextInt(), Character.MAX_RADIX) + "-test.lock"; Lock l = makeLock(randomLockName); try { l.obtain(); + l.release(); } catch (IOException e) { - IOException e2 = new IOException("Failed to acquire random test lock; please verify filesystem for lock directory '" + lockDir + "' supports locking"); + RuntimeException e2 = new RuntimeException("Failed to acquire random test lock; please verify filesystem for lock directory '" + lockDir + "' supports locking"); e2.initCause(e); throw e2; - } - - l.release(); + } } /** * Create a NativeFSLockFactory instance, with null (unset) - * lock directory. This is package-private and is only - * used by FSDirectory when creating this LockFactory via - * the System property - * org.apache.lucene.store.FSDirectoryLockFactoryClass. + * lock directory. When you pass this factory to a {@link FSDirectory} + * subclass, the lock directory is automatically set to the + * directory itsself. Be sure to create one instance for each directory + * your create! */ - NativeFSLockFactory() throws IOException { + public NativeFSLockFactory() throws IOException { this((File) null); } @@ -117,32 +122,10 @@ public class NativeFSLockFactory extends LockFactory { setLockDir(lockDir); } - /** - * Set the lock directory. This is package-private and is - * only used externally by FSDirectory when creating this - * LockFactory via the System property - * org.apache.lucene.store.FSDirectoryLockFactoryClass. - */ - void setLockDir(File lockDir) throws IOException { - this.lockDir = lockDir; - if (lockDir != null) { - // Ensure that lockDir exists and is a directory. - if (!lockDir.exists()) { - if (!lockDir.mkdirs()) - throw new IOException("Cannot create directory: " + - lockDir.getAbsolutePath()); - } else if (!lockDir.isDirectory()) { - throw new IOException("Found regular file where directory expected: " + - lockDir.getAbsolutePath()); - } - - acquireTestLock(); - } - } - public synchronized Lock makeLock(String lockName) { + acquireTestLock(); if (lockPrefix != null) - lockName = lockPrefix + "-n-" + lockName; + lockName = lockPrefix + "-" + lockName; return new NativeFSLock(lockDir, lockName); } @@ -153,7 +136,7 @@ public class NativeFSLockFactory extends LockFactory { // really want to see the files go away: if (lockDir.exists()) { if (lockPrefix != null) { - lockName = lockPrefix + "-n-" + lockName; + lockName = lockPrefix + "-" + lockName; } File lockFile = new File(lockDir, lockName); if (lockFile.exists() && !lockFile.delete()) { @@ -188,9 +171,13 @@ class NativeFSLock extends Lock { path = new File(lockDir, lockFileName); } + private synchronized boolean lockExists() { + return lock != null; + } + public synchronized boolean obtain() throws IOException { - if (isLocked()) { + if (lockExists()) { // Our instance is already locked: return false; } @@ -276,7 +263,7 @@ class NativeFSLock extends Lock { } } finally { - if (markedHeld && !isLocked()) { + if (markedHeld && !lockExists()) { synchronized(LOCK_HELD) { if (LOCK_HELD.contains(canonicalPath)) { LOCK_HELD.remove(canonicalPath); @@ -284,11 +271,11 @@ class NativeFSLock extends Lock { } } } - return isLocked(); + return lockExists(); } public synchronized void release() throws IOException { - if (isLocked()) { + if (lockExists()) { try { lock.release(); } finally { @@ -313,7 +300,22 @@ class NativeFSLock extends Lock { } public synchronized boolean isLocked() { - return lock != null; + // The test for is isLocked is not directly possible with native file locks: + + // First a shortcut, if a lock reference in this instance is available + if (lockExists()) return true; + + // Look if lock file is present; if not, there can definitely be no lock! + if (!path.exists()) return false; + + // Try to obtain and release (if was locked) the lock + try { + boolean obtained = obtain(); + if (obtained) release(); + return !obtained; + } catch (IOException ioe) { + return false; + } } public String toString() { diff --git a/src/java/org/apache/lucene/store/SimpleFSDirectory.java b/src/java/org/apache/lucene/store/SimpleFSDirectory.java index 31d0819ad99..d5cf22ed706 100644 --- a/src/java/org/apache/lucene/store/SimpleFSDirectory.java +++ b/src/java/org/apache/lucene/store/SimpleFSDirectory.java @@ -32,14 +32,15 @@ public class SimpleFSDirectory extends FSDirectory { /** Create a new SimpleFSDirectory for the named location. * * @param path the path of the directory - * @param lockFactory the lock factory to use, or null for the default. + * @param lockFactory the lock factory to use, or null for the default + * ({@link NativeFSLockFactory}); * @throws IOException */ public SimpleFSDirectory(File path, LockFactory lockFactory) throws IOException { super(path, lockFactory); } - /** Create a new SimpleFSDirectory for the named location and the default lock factory. + /** Create a new SimpleFSDirectory for the named location and {@link NativeFSLockFactory}. * * @param path the path of the directory * @throws IOException diff --git a/src/java/org/apache/lucene/store/SimpleFSLockFactory.java b/src/java/org/apache/lucene/store/SimpleFSLockFactory.java index 60da6f35150..8aa26b43b1a 100755 --- a/src/java/org/apache/lucene/store/SimpleFSLockFactory.java +++ b/src/java/org/apache/lucene/store/SimpleFSLockFactory.java @@ -22,8 +22,7 @@ import java.io.IOException; /** *

Implements {@link LockFactory} using {@link - * File#createNewFile()}. This is the default LockFactory - * for {@link FSDirectory}.

+ * File#createNewFile()}.

* *

NOTE: the javadocs @@ -52,24 +51,16 @@ import java.io.IOException; * @see LockFactory */ -public class SimpleFSLockFactory extends LockFactory { - - /** - * Directory specified by org.apache.lucene.lockDir - * system property. If that is not set, then java.io.tmpdir - * system property is used. - */ - - private File lockDir; +public class SimpleFSLockFactory extends FSLockFactory { /** * Create a SimpleFSLockFactory instance, with null (unset) - * lock directory. This is package-private and is only - * used by FSDirectory when creating this LockFactory via - * the System property - * org.apache.lucene.store.FSDirectoryLockFactoryClass. + * lock directory. When you pass this factory to a {@link FSDirectory} + * subclass, the lock directory is automatically set to the + * directory itsself. Be sure to create one instance for each directory + * your create! */ - SimpleFSLockFactory() throws IOException { + public SimpleFSLockFactory() throws IOException { this((File) null); } @@ -90,16 +81,6 @@ public class SimpleFSLockFactory extends LockFactory { setLockDir(lockDir); } - /** - * Set the lock directory. This is package-private and is - * only used externally by FSDirectory when creating this - * LockFactory via the System property - * org.apache.lucene.store.FSDirectoryLockFactoryClass. - */ - void setLockDir(File lockDir) throws IOException { - this.lockDir = lockDir; - } - public Lock makeLock(String lockName) { if (lockPrefix != null) { lockName = lockPrefix + "-" + lockName; diff --git a/src/test/org/apache/lucene/store/TestLockFactory.java b/src/test/org/apache/lucene/store/TestLockFactory.java index a31d33b4a44..cd9e5e6f789 100755 --- a/src/test/org/apache/lucene/store/TestLockFactory.java +++ b/src/test/org/apache/lucene/store/TestLockFactory.java @@ -381,26 +381,30 @@ public class TestLockFactory extends LuceneTestCase { assertTrue("failed to obtain 2nd lock after first one was freed", l2.obtain()); l2.release(); - // Make sure we can obtain first one again: + // Make sure we can obtain first one again, test isLocked(): assertTrue("failed to obtain lock", l.obtain()); + assertTrue(l.isLocked()); + assertTrue(l2.isLocked()); l.release(); + assertFalse(l.isLocked()); + assertFalse(l2.isLocked()); } - // Verify: NativeFSLockFactory assigns different lock - // prefixes to different directories: + // Verify: NativeFSLockFactory assigns null as lockPrefix if the lockDir is inside directory public void testNativeFSLockFactoryPrefix() throws IOException { - // Make sure we get identical instances: File fdir1 = _TestUtil.getTempDir("TestLockFactory.8"); + File fdir2 = _TestUtil.getTempDir("TestLockFactory.8.Lockdir"); Directory dir1 = FSDirectory.open(fdir1, new NativeFSLockFactory(fdir1)); - File fdir2 = _TestUtil.getTempDir("TestLockFactory.9"); - Directory dir2 = FSDirectory.open(fdir2, new NativeFSLockFactory(fdir2)); + // same directory, but locks are stored somewhere else. The prefix of the lock factory should != null + Directory dir2 = FSDirectory.open(fdir1, new NativeFSLockFactory(fdir2)); String prefix1 = dir1.getLockFactory().getLockPrefix(); + assertNull("Lock prefix for lockDir same as directory should be null", prefix1); + String prefix2 = dir2.getLockFactory().getLockPrefix(); + assertNotNull("Lock prefix for lockDir outside of directory should be not null", prefix2); - assertTrue("Native Lock Factories are incorrectly shared: dir1 and dir2 have same lock prefix '" + prefix1 + "'; they should be different", - !prefix1.equals(prefix2)); _TestUtil.rmDir(fdir1); _TestUtil.rmDir(fdir2); }