LUCENE-7373: deprecate Directory.renameFile, which both renamed and fsync'd the directory, replacing it with separate rename and syncMetaData methods

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)

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(); }

lucene/core/src/java/org/apache/lucene/codecs/lucene50/Lucene50CompoundReader.java

@@ -168,10 +168,15 @@ 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 */
   @Override

lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java

@@ -788,7 +788,8 @@ public final class SegmentInfos implements Cloneable, Iterable<SegmentCommitInfo
     try {
       final String src = IndexFileNames.fileNameFromGeneration(IndexFileNames.PENDING_SEGMENTS, "", generation);
       dest = IndexFileNames.fileNameFromGeneration(IndexFileNames.SEGMENTS, "", generation);
-      dir.renameFile(src, dest);
+      dir.rename(src, dest);
+      dir.syncMetaData();
       success = true;
     } finally {
       if (!success) {

lucene/core/src/java/org/apache/lucene/store/Directory.java

@@ -99,8 +99,32 @@ public abstract class Directory implements Closeable {
    * 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.
+   *
+   * @deprecated Use {@link #rename} and {@link #syncMetaData} instead.
    */
-  public abstract void renameFile(String source, String dest) throws IOException;
+  @Deprecated
+  public final void renameFile(String source, String dest) throws IOException {
+    rename(source, dest);
+    syncMetaData();
+  }
+
+  /**
+   * Renames {@code source} to {@code dest} as an atomic operation,
+   * where {@code dest} does not yet exist in the directory.
+   * <p>
+   * 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.
    * <p>Throws {@link FileNotFoundException} or {@link NoSuchFileException}

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();
   }

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

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

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

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

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 {

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);

lucene/core/src/test/org/apache/lucene/store/TestFilterDirectory.java

@@ -39,6 +39,7 @@ public class TestFilterDirectory extends BaseDirectoryTestCase {
     Set<Method> 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));

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());
   }
   

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 {

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 {

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();

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();

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:

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<String> 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<String> filesToCorrupt = new ArrayList<>(files);

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));

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());