From df9efb8b6da5195d6c8454cb7fd9b91147cb93fd Mon Sep 17 00:00:00 2001 From: Mike McCandless Date: Mon, 11 Jul 2016 11:15:22 -0400 Subject: [PATCH] LUCENE-7373: deprecate Directory.renameFile, which both renamed and fsync'd the directory, replacing it with separate rename and syncMetaData methods --- lucene/CHANGES.txt | 5 ++++ .../simpletext/SimpleTextCompoundFormat.java | 5 +++- .../lucene50/Lucene50CompoundReader.java | 7 ++++- .../org/apache/lucene/index/SegmentInfos.java | 3 ++- .../org/apache/lucene/store/Directory.java | 26 ++++++++++++++++++- .../org/apache/lucene/store/FSDirectory.java | 11 +++++--- .../lucene/store/FileSwitchDirectory.java | 10 +++++-- .../apache/lucene/store/FilterDirectory.java | 9 +++++-- .../store/LockValidatingDirectoryWrapper.java | 10 +++++-- .../lucene/store/NRTCachingDirectory.java | 4 +-- .../org/apache/lucene/store/RAMDirectory.java | 7 ++++- .../store/TrackingDirectoryWrapper.java | 4 +-- .../lucene/store/TestFilterDirectory.java | 1 + .../store/TestTrackingDirectoryWrapper.java | 2 +- .../IndexAndTaxonomyReplicationHandler.java | 6 +++-- .../replicator/IndexReplicationHandler.java | 3 ++- .../lucene/replicator/nrt/SimpleCopyJob.java | 2 +- .../index/BaseCompoundFormatTestCase.java | 2 +- .../lucene/store/BaseDirectoryTestCase.java | 4 +-- .../lucene/store/MockDirectoryWrapper.java | 18 +++++++++++-- .../apache/solr/store/hdfs/HdfsDirectory.java | 7 ++++- .../solr/store/hdfs/HdfsDirectoryTest.java | 2 +- 22 files changed, 118 insertions(+), 30 deletions(-) diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index bed9b80601e..dfc9ebf4632 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -41,6 +41,11 @@ New Features analyzer for the Ukrainian language (Andriy Rysin via Mike McCandless) +* LUCENE-7373: Directory.renameFile, which did both renaming and fsync + of the directory metadata, has been deprecated; use the new separate + methods Directory.rename and Directory.syncMetaData instead (Robert Muir, + Uwe Schindler, Mike McCandless) + Bug Fixes * LUCENE-6662: Fixed potential resource leaks. (Rishabh Patel via Adrien Grand) diff --git a/lucene/codecs/src/java/org/apache/lucene/codecs/simpletext/SimpleTextCompoundFormat.java b/lucene/codecs/src/java/org/apache/lucene/codecs/simpletext/SimpleTextCompoundFormat.java index 4f9cfbffece..bed0c07a5ab 100644 --- a/lucene/codecs/src/java/org/apache/lucene/codecs/simpletext/SimpleTextCompoundFormat.java +++ b/lucene/codecs/src/java/org/apache/lucene/codecs/simpletext/SimpleTextCompoundFormat.java @@ -151,7 +151,10 @@ public class SimpleTextCompoundFormat extends CompoundFormat { public void deleteFile(String name) { throw new UnsupportedOperationException(); } @Override - public void renameFile(String source, String dest) { throw new UnsupportedOperationException(); } + public void rename(String source, String dest) { throw new UnsupportedOperationException(); } + + @Override + public void syncMetaData() { throw new UnsupportedOperationException(); } @Override public Lock obtainLock(String name) { throw new UnsupportedOperationException(); } diff --git a/lucene/core/src/java/org/apache/lucene/codecs/lucene50/Lucene50CompoundReader.java b/lucene/core/src/java/org/apache/lucene/codecs/lucene50/Lucene50CompoundReader.java index f7de16915dc..7526c88c202 100644 --- a/lucene/core/src/java/org/apache/lucene/codecs/lucene50/Lucene50CompoundReader.java +++ b/lucene/core/src/java/org/apache/lucene/codecs/lucene50/Lucene50CompoundReader.java @@ -168,9 +168,14 @@ final class Lucene50CompoundReader extends Directory { /** Not implemented * @throws UnsupportedOperationException always: not supported by CFS */ - public void renameFile(String from, String to) { + @Override + public void rename(String from, String to) { throw new UnsupportedOperationException(); } + + @Override + public void syncMetaData() { + } /** Returns the length of a file in the directory. * @throws IOException if the file does not exist */ diff --git a/lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java b/lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java index 6b48e5d2edd..d87fc8409ab 100644 --- a/lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java +++ b/lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java @@ -788,7 +788,8 @@ public final class SegmentInfos implements Cloneable, Iterable + * Notes: This method is used by IndexWriter to publish commits. + * It is ok if this operation is not truly atomic, for example + * both {@code source} and {@code dest} can be visible temporarily. + * It is just important that the contents of {@code dest} appear + * atomically, or an exception is thrown. + */ + public abstract void rename(String source, String dest) throws IOException; + + /** + * Ensure that directory metadata, such as recent file renames, are made + * durable. + */ + public abstract void syncMetaData() throws IOException; /** Returns a stream reading an existing file. *

Throws {@link FileNotFoundException} or {@link NoSuchFileException} diff --git a/lucene/core/src/java/org/apache/lucene/store/FSDirectory.java b/lucene/core/src/java/org/apache/lucene/store/FSDirectory.java index 0721ae602c0..50b52ceb07a 100644 --- a/lucene/core/src/java/org/apache/lucene/store/FSDirectory.java +++ b/lucene/core/src/java/org/apache/lucene/store/FSDirectory.java @@ -288,15 +288,20 @@ public abstract class FSDirectory extends BaseDirectory { } @Override - public void renameFile(String source, String dest) throws IOException { + public void rename(String source, String dest) throws IOException { ensureOpen(); if (pendingDeletes.contains(source)) { throw new NoSuchFileException("file \"" + source + "\" is pending delete and cannot be moved"); } pendingDeletes.remove(dest); Files.move(directory.resolve(source), directory.resolve(dest), StandardCopyOption.ATOMIC_MOVE); - // TODO: should we move directory fsync to a separate 'syncMetadata' method? - // for example, to improve listCommits(), IndexFileDeleter could also call that after deleting segments_Ns + maybeDeletePendingFiles(); + } + + @Override + public void syncMetaData() throws IOException { + // TODO: to improve listCommits(), IndexFileDeleter could call this after deleting segments_Ns + ensureOpen(); IOUtils.fsync(directory, true); maybeDeletePendingFiles(); } diff --git a/lucene/core/src/java/org/apache/lucene/store/FileSwitchDirectory.java b/lucene/core/src/java/org/apache/lucene/store/FileSwitchDirectory.java index db9f08542d9..f0c46b1a4ea 100644 --- a/lucene/core/src/java/org/apache/lucene/store/FileSwitchDirectory.java +++ b/lucene/core/src/java/org/apache/lucene/store/FileSwitchDirectory.java @@ -182,14 +182,20 @@ public class FileSwitchDirectory extends Directory { } @Override - public void renameFile(String source, String dest) throws IOException { + public void rename(String source, String dest) throws IOException { Directory sourceDir = getDirectory(source); // won't happen with standard lucene index files since pending and commit will // always have the same extension ("") if (sourceDir != getDirectory(dest)) { throw new AtomicMoveNotSupportedException(source, dest, "source and dest are in different directories"); } - sourceDir.renameFile(source, dest); + sourceDir.rename(source, dest); + } + + @Override + public void syncMetaData() throws IOException { + primaryDir.syncMetaData(); + secondaryDir.syncMetaData(); } @Override diff --git a/lucene/core/src/java/org/apache/lucene/store/FilterDirectory.java b/lucene/core/src/java/org/apache/lucene/store/FilterDirectory.java index 8148b5ac3a3..897ce768a86 100644 --- a/lucene/core/src/java/org/apache/lucene/store/FilterDirectory.java +++ b/lucene/core/src/java/org/apache/lucene/store/FilterDirectory.java @@ -84,8 +84,13 @@ public abstract class FilterDirectory extends Directory { } @Override - public void renameFile(String source, String dest) throws IOException { - in.renameFile(source, dest); + public void rename(String source, String dest) throws IOException { + in.rename(source, dest); + } + + @Override + public void syncMetaData() throws IOException { + in.syncMetaData(); } @Override diff --git a/lucene/core/src/java/org/apache/lucene/store/LockValidatingDirectoryWrapper.java b/lucene/core/src/java/org/apache/lucene/store/LockValidatingDirectoryWrapper.java index f6c88675337..d2617207d0c 100644 --- a/lucene/core/src/java/org/apache/lucene/store/LockValidatingDirectoryWrapper.java +++ b/lucene/core/src/java/org/apache/lucene/store/LockValidatingDirectoryWrapper.java @@ -51,9 +51,15 @@ public final class LockValidatingDirectoryWrapper extends FilterDirectory { } @Override - public void renameFile(String source, String dest) throws IOException { + public void rename(String source, String dest) throws IOException { writeLock.ensureValid(); - in.renameFile(source, dest); + in.rename(source, dest); + } + + @Override + public void syncMetaData() throws IOException { + writeLock.ensureValid(); + in.syncMetaData(); } @Override diff --git a/lucene/core/src/java/org/apache/lucene/store/NRTCachingDirectory.java b/lucene/core/src/java/org/apache/lucene/store/NRTCachingDirectory.java index 9be0b9e9c91..c2b071ba234 100644 --- a/lucene/core/src/java/org/apache/lucene/store/NRTCachingDirectory.java +++ b/lucene/core/src/java/org/apache/lucene/store/NRTCachingDirectory.java @@ -170,12 +170,12 @@ public class NRTCachingDirectory extends FilterDirectory implements Accountable } @Override - public void renameFile(String source, String dest) throws IOException { + public void rename(String source, String dest) throws IOException { unCache(source); if (cache.fileNameExists(dest)) { throw new IllegalArgumentException("target file " + dest + " already exists"); } - in.renameFile(source, dest); + in.rename(source, dest); } @Override diff --git a/lucene/core/src/java/org/apache/lucene/store/RAMDirectory.java b/lucene/core/src/java/org/apache/lucene/store/RAMDirectory.java index 54375e66c03..3c2bed28941 100644 --- a/lucene/core/src/java/org/apache/lucene/store/RAMDirectory.java +++ b/lucene/core/src/java/org/apache/lucene/store/RAMDirectory.java @@ -214,7 +214,7 @@ public class RAMDirectory extends BaseDirectory implements Accountable { } @Override - public void renameFile(String source, String dest) throws IOException { + public void rename(String source, String dest) throws IOException { ensureOpen(); RAMFile file = fileMap.get(source); if (file == null) { @@ -229,6 +229,11 @@ public class RAMDirectory extends BaseDirectory implements Accountable { fileMap.remove(source); } + @Override + public void syncMetaData() throws IOException { + // we are by definition not durable! + } + /** Returns a stream reading an existing file. */ @Override public IndexInput openInput(String name, IOContext context) throws IOException { diff --git a/lucene/core/src/java/org/apache/lucene/store/TrackingDirectoryWrapper.java b/lucene/core/src/java/org/apache/lucene/store/TrackingDirectoryWrapper.java index d78a5b6464d..4813ee2512a 100644 --- a/lucene/core/src/java/org/apache/lucene/store/TrackingDirectoryWrapper.java +++ b/lucene/core/src/java/org/apache/lucene/store/TrackingDirectoryWrapper.java @@ -60,8 +60,8 @@ public final class TrackingDirectoryWrapper extends FilterDirectory { } @Override - public void renameFile(String source, String dest) throws IOException { - in.renameFile(source, dest); + public void rename(String source, String dest) throws IOException { + in.rename(source, dest); synchronized (createdFileNames) { createdFileNames.add(dest); createdFileNames.remove(source); diff --git a/lucene/core/src/test/org/apache/lucene/store/TestFilterDirectory.java b/lucene/core/src/test/org/apache/lucene/store/TestFilterDirectory.java index 6224140e3ca..8085462be08 100644 --- a/lucene/core/src/test/org/apache/lucene/store/TestFilterDirectory.java +++ b/lucene/core/src/test/org/apache/lucene/store/TestFilterDirectory.java @@ -39,6 +39,7 @@ public class TestFilterDirectory extends BaseDirectoryTestCase { Set exclude = new HashSet<>(); exclude.add(Directory.class.getMethod("copyFrom", Directory.class, String.class, String.class, IOContext.class)); exclude.add(Directory.class.getMethod("openChecksumInput", String.class, IOContext.class)); + exclude.add(Directory.class.getMethod("renameFile", String.class, String.class)); for (Method m : FilterDirectory.class.getMethods()) { if (m.getDeclaringClass() == Directory.class) { assertTrue("method " + m.getName() + " not overridden!", exclude.contains(m)); diff --git a/lucene/core/src/test/org/apache/lucene/store/TestTrackingDirectoryWrapper.java b/lucene/core/src/test/org/apache/lucene/store/TestTrackingDirectoryWrapper.java index a9574ce330e..008ac748092 100644 --- a/lucene/core/src/test/org/apache/lucene/store/TestTrackingDirectoryWrapper.java +++ b/lucene/core/src/test/org/apache/lucene/store/TestTrackingDirectoryWrapper.java @@ -51,7 +51,7 @@ public class TestTrackingDirectoryWrapper extends BaseDirectoryTestCase { TrackingDirectoryWrapper dir = new TrackingDirectoryWrapper(new RAMDirectory()); dir.createOutput("foo", newIOContext(random())).close(); assertEquals(asSet("foo"), dir.getCreatedFiles()); - dir.renameFile("foo", "bar"); + dir.rename("foo", "bar"); assertEquals(asSet("bar"), dir.getCreatedFiles()); } diff --git a/lucene/replicator/src/java/org/apache/lucene/replicator/IndexAndTaxonomyReplicationHandler.java b/lucene/replicator/src/java/org/apache/lucene/replicator/IndexAndTaxonomyReplicationHandler.java index ade091c4b4d..11ea15db6c2 100644 --- a/lucene/replicator/src/java/org/apache/lucene/replicator/IndexAndTaxonomyReplicationHandler.java +++ b/lucene/replicator/src/java/org/apache/lucene/replicator/IndexAndTaxonomyReplicationHandler.java @@ -141,10 +141,12 @@ public class IndexAndTaxonomyReplicationHandler implements ReplicationHandler { indexDir.sync(Collections.singletonList(indexPendingFile)); if (taxoSegmentsFile != null) { - taxoDir.renameFile(taxoPendingFile, taxoSegmentsFile); + taxoDir.rename(taxoPendingFile, taxoSegmentsFile); + taxoDir.syncMetaData(); } - indexDir.renameFile(indexPendingFile, indexSegmentsFile); + indexDir.rename(indexPendingFile, indexSegmentsFile); + indexDir.syncMetaData(); success = true; } finally { diff --git a/lucene/replicator/src/java/org/apache/lucene/replicator/IndexReplicationHandler.java b/lucene/replicator/src/java/org/apache/lucene/replicator/IndexReplicationHandler.java index 8f801a33f88..fbb4a26c4a6 100644 --- a/lucene/replicator/src/java/org/apache/lucene/replicator/IndexReplicationHandler.java +++ b/lucene/replicator/src/java/org/apache/lucene/replicator/IndexReplicationHandler.java @@ -233,7 +233,8 @@ public class IndexReplicationHandler implements ReplicationHandler { // now copy and fsync segmentsFile as pending, then rename (simulating lucene commit) indexDir.copyFrom(clientDir, segmentsFile, pendingSegmentsFile, IOContext.READONCE); indexDir.sync(Collections.singletonList(pendingSegmentsFile)); - indexDir.renameFile(pendingSegmentsFile, segmentsFile); + indexDir.rename(pendingSegmentsFile, segmentsFile); + indexDir.syncMetaData(); success = true; } finally { diff --git a/lucene/replicator/src/test/org/apache/lucene/replicator/nrt/SimpleCopyJob.java b/lucene/replicator/src/test/org/apache/lucene/replicator/nrt/SimpleCopyJob.java index cee74ce308a..6793df866f2 100644 --- a/lucene/replicator/src/test/org/apache/lucene/replicator/nrt/SimpleCopyJob.java +++ b/lucene/replicator/src/test/org/apache/lucene/replicator/nrt/SimpleCopyJob.java @@ -145,7 +145,7 @@ class SimpleCopyJob extends CopyJob { // NOTE: if this throws exception, then some files have been moved to their true names, and others are leftover .tmp files. I don't // think heroic exception handling is necessary (no harm will come, except some leftover files), nor warranted here (would make the // code more complex, for the exceptional cases when something is wrong w/ your IO system): - dest.dir.renameFile(tmpFileName, fileName); + dest.dir.rename(tmpFileName, fileName); } copiedFiles.clear(); diff --git a/lucene/test-framework/src/java/org/apache/lucene/index/BaseCompoundFormatTestCase.java b/lucene/test-framework/src/java/org/apache/lucene/index/BaseCompoundFormatTestCase.java index 256b24e7d02..30788e84065 100644 --- a/lucene/test-framework/src/java/org/apache/lucene/index/BaseCompoundFormatTestCase.java +++ b/lucene/test-framework/src/java/org/apache/lucene/index/BaseCompoundFormatTestCase.java @@ -272,7 +272,7 @@ public abstract class BaseCompoundFormatTestCase extends BaseIndexFileFormatTest si.getCodec().compoundFormat().write(dir, si, IOContext.DEFAULT); Directory cfs = si.getCodec().compoundFormat().getCompoundReader(dir, si, IOContext.DEFAULT); expectThrows(UnsupportedOperationException.class, () -> { - cfs.renameFile(testfile, "bogus"); + cfs.rename(testfile, "bogus"); }); cfs.close(); diff --git a/lucene/test-framework/src/java/org/apache/lucene/store/BaseDirectoryTestCase.java b/lucene/test-framework/src/java/org/apache/lucene/store/BaseDirectoryTestCase.java index e5af03b69b2..1aae60bcc71 100644 --- a/lucene/test-framework/src/java/org/apache/lucene/store/BaseDirectoryTestCase.java +++ b/lucene/test-framework/src/java/org/apache/lucene/store/BaseDirectoryTestCase.java @@ -110,7 +110,7 @@ public abstract class BaseDirectoryTestCase extends LuceneTestCase { output.writeBytes(bytes, bytes.length); output.close(); - dir.renameFile("foobar", "foobaz"); + dir.rename("foobar", "foobaz"); IndexInput input = dir.openInput("foobaz", newIOContext(random())); byte bytes2[] = new byte[numBytes]; @@ -1196,7 +1196,7 @@ public abstract class BaseDirectoryTestCase extends LuceneTestCase { // Make sure rename fails: expectThrows(NoSuchFileException.class, () -> { - fsDir.renameFile(fileName, "file2"); + fsDir.rename(fileName, "file2"); }); // Make sure delete fails: diff --git a/lucene/test-framework/src/java/org/apache/lucene/store/MockDirectoryWrapper.java b/lucene/test-framework/src/java/org/apache/lucene/store/MockDirectoryWrapper.java index 4f7de29b70f..e78968d452a 100644 --- a/lucene/test-framework/src/java/org/apache/lucene/store/MockDirectoryWrapper.java +++ b/lucene/test-framework/src/java/org/apache/lucene/store/MockDirectoryWrapper.java @@ -208,7 +208,7 @@ public class MockDirectoryWrapper extends BaseDirectoryWrapper { } @Override - public synchronized void renameFile(String source, String dest) throws IOException { + public synchronized void rename(String source, String dest) throws IOException { maybeYield(); maybeThrowDeterministicException(); @@ -226,7 +226,7 @@ public class MockDirectoryWrapper extends BaseDirectoryWrapper { boolean success = false; try { - in.renameFile(source, dest); + in.rename(source, dest); success = true; } finally { if (success) { @@ -242,6 +242,16 @@ public class MockDirectoryWrapper extends BaseDirectoryWrapper { } } + @Override + public synchronized void syncMetaData() throws IOException { + maybeYield(); + maybeThrowDeterministicException(); + if (crashed) { + throw new IOException("cannot rename after crash"); + } + in.syncMetaData(); + } + public synchronized final long sizeInBytes() throws IOException { if (in instanceof RAMDirectory) return ((RAMDirectory) in).ramBytesUsed(); @@ -303,6 +313,10 @@ public class MockDirectoryWrapper extends BaseDirectoryWrapper { } private synchronized void _corruptFiles(Collection files) throws IOException { + + // TODO: we should also mess with any recent file renames, file deletions, if + // syncMetaData was not called!! + // Must make a copy because we change the incoming unsyncedFiles // when we create temp files, delete, etc., below: final List filesToCorrupt = new ArrayList<>(files); diff --git a/solr/core/src/java/org/apache/solr/store/hdfs/HdfsDirectory.java b/solr/core/src/java/org/apache/solr/store/hdfs/HdfsDirectory.java index 0a2569210bb..72d48aea9bd 100644 --- a/solr/core/src/java/org/apache/solr/store/hdfs/HdfsDirectory.java +++ b/solr/core/src/java/org/apache/solr/store/hdfs/HdfsDirectory.java @@ -148,12 +148,17 @@ public class HdfsDirectory extends BaseDirectory { } @Override - public void renameFile(String source, String dest) throws IOException { + public void rename(String source, String dest) throws IOException { Path sourcePath = new Path(hdfsDirPath, source); Path destPath = new Path(hdfsDirPath, dest); fileContext.rename(sourcePath, destPath); } + @Override + public void syncMetaData() throws IOException { + // TODO: how? + } + @Override public long fileLength(String name) throws IOException { FileStatus fileStatus = fileSystem.getFileStatus(new Path(hdfsDirPath, name)); diff --git a/solr/core/src/test/org/apache/solr/store/hdfs/HdfsDirectoryTest.java b/solr/core/src/test/org/apache/solr/store/hdfs/HdfsDirectoryTest.java index fab6efd6f49..0d5d0a78eb7 100644 --- a/solr/core/src/test/org/apache/solr/store/hdfs/HdfsDirectoryTest.java +++ b/solr/core/src/test/org/apache/solr/store/hdfs/HdfsDirectoryTest.java @@ -128,7 +128,7 @@ public class HdfsDirectoryTest extends SolrTestCaseJ4 { IndexOutput output = directory.createOutput("testing.test", new IOContext()); output.writeInt(12345); output.close(); - directory.renameFile("testing.test", "testing.test.renamed"); + directory.rename("testing.test", "testing.test.renamed"); assertFalse(slowFileExists(directory, "testing.test")); assertTrue(slowFileExists(directory, "testing.test.renamed")); IndexInput input = directory.openInput("testing.test.renamed", new IOContext());