SOLR-9642: Refactor the snapshot cleanup mechanism to rely on Lucene

The current snapshot cleanup mechanism is based on reference counting
the index files shared between multiple segments. Since this mechanism
completely skips the Lucene APIs, it is not portable (e.g. it doesn't
work on 4.10.x version).

This patch provides an alternate implementation which relies exclusively
on Lucene IndexWriter (+ IndexDeletionPolicy) for cleanup.

mend

solr/CHANGES.txt

@@ -279,6 +279,8 @@ Other Changes
 
 * SOLR-9625: Add HelloWorldSolrCloudTestCase class (Christine Poerschke, Alan Woodward, Alexandre Rafalovitch)
 
+* SOLR-9642: Refactor the core level snapshot cleanup mechanism to rely on Lucene (Hrishikesh Gadre via yonik)
+
 ==================  6.2.1 ==================
 
 Bug Fixes

solr/core/src/java/org/apache/solr/core/SolrCore.java

@@ -39,6 +39,7 @@
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Properties;
 import java.util.Set;
 import java.util.concurrent.Callable;
@@ -81,7 +82,9 @@
 import org.apache.solr.common.util.SimpleOrderedMap;
 import org.apache.solr.common.util.Utils;
 import org.apache.solr.core.DirectoryFactory.DirContext;
+import org.apache.solr.core.snapshots.SolrSnapshotManager;
 import org.apache.solr.core.snapshots.SolrSnapshotMetaDataManager;
+import org.apache.solr.core.snapshots.SolrSnapshotMetaDataManager.SnapshotMetaData;
 import org.apache.solr.handler.IndexFetcher;
 import org.apache.solr.handler.ReplicationHandler;
 import org.apache.solr.handler.RequestHandlerBase;
@@ -194,6 +197,7 @@ public final class SolrCore implements SolrInfoMBean, Closeable {
   private final List<Runnable> confListeners = new CopyOnWriteArrayList<>();
 
   private final ReentrantLock ruleExpiryLock;
+  private final ReentrantLock snapshotDelLock; // A lock instance to guard against concurrent deletions.
 
   public Date getStartTimeStamp() { return startTime; }
 
@@ -431,6 +435,83 @@ private SolrSnapshotMetaDataManager initSnapshotMetaDataManager() {
     }
   }
 
+  /**
+   * This method deletes the snapshot with the specified name. If the directory
+   * storing the snapshot is not the same as the *current* core index directory,
+   * then delete the files corresponding to this snapshot. Otherwise we leave the
+   * index files related to snapshot as is (assuming the underlying Solr IndexDeletionPolicy
+   * will clean them up appropriately).
+   *
+   * @param commitName The name of the snapshot to be deleted.
+   * @throws IOException in case of I/O error.
+   */
+  public void deleteNamedSnapshot(String commitName) throws IOException {
+    // Note this lock is required to prevent multiple snapshot deletions from
+    // opening multiple IndexWriter instances simultaneously.
+    this.snapshotDelLock.lock();
+    try {
+      Optional<SnapshotMetaData> metadata = snapshotMgr.release(commitName);
+      if (metadata.isPresent()) {
+        long gen = metadata.get().getGenerationNumber();
+        String indexDirPath = metadata.get().getIndexDirPath();
+
+        if (!indexDirPath.equals(getIndexDir())) {
+          Directory d = getDirectoryFactory().get(indexDirPath, DirContext.DEFAULT, "none");
+          try {
+            Collection<SnapshotMetaData> snapshots = snapshotMgr.listSnapshotsInIndexDir(indexDirPath);
+            log.info("Following snapshots exist in the index directory {} : {}", indexDirPath, snapshots);
+            if (snapshots.isEmpty()) {// No snapshots remain in this directory. Can be cleaned up!
+              log.info("Removing index directory {} since all named snapshots are deleted.", indexDirPath);
+              getDirectoryFactory().remove(d);
+            } else {
+              SolrSnapshotManager.deleteSnapshotIndexFiles(this, d, gen);
+            }
+          } finally {
+            getDirectoryFactory().release(d);
+          }
+        }
+      }
+    } finally {
+      snapshotDelLock.unlock();
+    }
+  }
+
+  /**
+   * This method deletes the index files not associated with any named snapshot only
+   * if the specified indexDirPath is not the *current* index directory.
+   *
+   * @param indexDirPath The path of the directory
+   * @throws IOException In case of I/O error.
+   */
+  public void deleteNonSnapshotIndexFiles(String indexDirPath) throws IOException {
+    // Skip if the specified indexDirPath is the *current* index directory.
+    if (getIndexDir().equals(indexDirPath)) {
+      return;
+    }
+
+    // Note this lock is required to prevent multiple snapshot deletions from
+    // opening multiple IndexWriter instances simultaneously.
+    this.snapshotDelLock.lock();
+    Directory dir = getDirectoryFactory().get(indexDirPath, DirContext.DEFAULT, "none");
+    try {
+      Collection<SnapshotMetaData> snapshots = snapshotMgr.listSnapshotsInIndexDir(indexDirPath);
+      log.info("Following snapshots exist in the index directory {} : {}", indexDirPath, snapshots);
+      // Delete the old index directory only if no snapshot exists in that directory.
+      if (snapshots.isEmpty()) {
+        log.info("Removing index directory {} since all named snapshots are deleted.", indexDirPath);
+        getDirectoryFactory().remove(dir);
+      } else {
+        SolrSnapshotManager.deleteNonSnapshotIndexFiles(this, dir, snapshots);
+      }
+    } finally {
+      snapshotDelLock.unlock();
+      if (dir != null) {
+        getDirectoryFactory().release(dir);
+      }
+    }
+  }
+
+
   private void initListeners() {
     final Class<SolrEventListener> clazz = SolrEventListener.class;
     final String label = "Event Listener";
@@ -863,6 +944,8 @@ public SolrCore(String name, String dataDir, SolrConfig config,
     bufferUpdatesIfConstructing(coreDescriptor);
 
     this.ruleExpiryLock = new ReentrantLock();
+    this.snapshotDelLock = new ReentrantLock();
+
     registerConfListener();
 
     assert ObjectReleaseTracker.track(this);

solr/core/src/java/org/apache/solr/core/snapshots/SolrSnapshotManager.java

@@ -19,18 +19,18 @@
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
 import java.util.Collection;
-import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
-import java.util.function.Function;
-import java.util.stream.Collectors;
-
-import com.google.common.annotations.VisibleForTesting;
-import org.apache.lucene.index.DirectoryReader;
 import org.apache.lucene.index.IndexCommit;
+import org.apache.lucene.index.IndexDeletionPolicy;
+import org.apache.lucene.index.IndexWriterConfig;
+import org.apache.lucene.index.IndexWriterConfig.OpenMode;
+import org.apache.lucene.index.NoMergePolicy;
 import org.apache.lucene.store.Directory;
+import org.apache.solr.core.SolrCore;
 import org.apache.solr.core.snapshots.SolrSnapshotMetaDataManager.SnapshotMetaData;
+import org.apache.solr.update.SolrIndexWriter;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -43,92 +43,78 @@ public class SolrSnapshotManager {
   /**
    * This method deletes index files of the {@linkplain IndexCommit} for the specified generation number.
    *
+   * @param core The Solr core
    * @param dir The index directory storing the snapshot.
-   * @param gen The generation number for the {@linkplain IndexCommit}
+   * @param gen The generation number of the {@linkplain IndexCommit} to be deleted.
    * @throws IOException in case of I/O errors.
    */
-  public static void deleteIndexFiles ( Directory dir, Collection<SnapshotMetaData> snapshots, long gen ) throws IOException {
-    List<IndexCommit> commits = DirectoryReader.listCommits(dir);
-    Map<String, Integer> refCounts = buildRefCounts(snapshots, commits);
-    for (IndexCommit ic : commits) {
-      if (ic.getGeneration() == gen) {
-        deleteIndexFiles(dir,refCounts, ic);
-        break;
+  public static void deleteSnapshotIndexFiles(SolrCore core, Directory dir, final long gen) throws IOException {
+    deleteSnapshotIndexFiles(core, dir, new IndexDeletionPolicy() {
+      @Override
+      public void onInit(List<? extends IndexCommit> commits) throws IOException {
+        for (IndexCommit ic : commits) {
+          if (gen == ic.getGeneration()) {
+            log.info("Deleting non-snapshotted index commit with generation {}", ic.getGeneration());
+            ic.delete();
+          }
+        }
       }
-    }
+
+      @Override
+      public void onCommit(List<? extends IndexCommit> commits)
+          throws IOException {}
+    });
   }
 
   /**
-   * This method deletes all files not corresponding to a configured snapshot in the specified index directory.
+   * This method deletes index files not associated with the specified <code>snapshots</code>.
    *
-   * @param dir The index directory to search for.
+   * @param core The Solr core
+   * @param dir The index directory storing the snapshot.
+   * @param snapshots The snapshots to be preserved.
    * @throws IOException in case of I/O errors.
    */
-  public static void deleteNonSnapshotIndexFiles (Directory dir, Collection<SnapshotMetaData> snapshots) throws IOException {
-    List<IndexCommit> commits = DirectoryReader.listCommits(dir);
-    Map<String, Integer> refCounts = buildRefCounts(snapshots, commits);
-    Set<Long> snapshotGenNumbers = snapshots.stream()
-                                            .map(SnapshotMetaData::getGenerationNumber)
-                                            .collect(Collectors.toSet());
-    for (IndexCommit ic : commits) {
-      if (!snapshotGenNumbers.contains(ic.getGeneration())) {
-        deleteIndexFiles(dir,refCounts, ic);
-      }
+  public static void deleteNonSnapshotIndexFiles(SolrCore core, Directory dir, Collection<SnapshotMetaData> snapshots) throws IOException {
+    final Set<Long> genNumbers = new HashSet<>();
+    for (SnapshotMetaData m : snapshots) {
+      genNumbers.add(m.getGenerationNumber());
     }
-  }
 
-  /**
-   * This method computes reference count for the index files by taking into consideration
-   * (a) configured snapshots and (b) files sharing between two or more {@linkplain IndexCommit} instances.
-   *
-   * @param snapshots A collection of user configured snapshots
-   * @param commits A list of {@linkplain IndexCommit} instances
-   * @return A map containing reference count for each index file referred in one of the {@linkplain IndexCommit} instances.
-   * @throws IOException in case of I/O error.
-   */
-  @VisibleForTesting
-  static Map<String, Integer> buildRefCounts (Collection<SnapshotMetaData> snapshots, List<IndexCommit> commits) throws IOException {
-    Map<String, Integer> result = new HashMap<>();
-    Map<Long, IndexCommit> commitsByGen = commits.stream().collect(
-        Collectors.toMap(IndexCommit::getGeneration, Function.identity()));
-
-    for(SnapshotMetaData md : snapshots) {
-      IndexCommit ic = commitsByGen.get(md.getGenerationNumber());
-      if (ic != null) {
-        Collection<String> fileNames = ic.getFileNames();
-        for(String fileName : fileNames) {
-          int refCount = result.getOrDefault(fileName, 0);
-          result.put(fileName, refCount+1);
+    deleteSnapshotIndexFiles(core, dir, new IndexDeletionPolicy() {
+      @Override
+      public void onInit(List<? extends IndexCommit> commits) throws IOException {
+        for (IndexCommit ic : commits) {
+          if (!genNumbers.contains(ic.getGeneration())) {
+            log.info("Deleting non-snapshotted index commit with generation {}", ic.getGeneration());
+            ic.delete();
+          }
         }
       }
-    }
 
-    return result;
+      @Override
+      public void onCommit(List<? extends IndexCommit> commits)
+          throws IOException {}
+    });
   }
 
   /**
-   * This method deletes the index files associated with specified <code>indexCommit</code> provided they
-   * are not referred by some other {@linkplain IndexCommit}.
+   * This method deletes index files of the {@linkplain IndexCommit} for the specified generation number.
    *
-   * @param dir The index directory containing the {@linkplain IndexCommit} to be deleted.
-   * @param refCounts A map containing reference counts for each file associated with every {@linkplain IndexCommit}
-   *                  in the specified directory.
-   * @param indexCommit The {@linkplain IndexCommit} whose files need to be deleted.
+   * @param core The Solr core
+   * @param dir The index directory storing the snapshot.
    * @throws IOException in case of I/O errors.
    */
-  private static void deleteIndexFiles ( Directory dir, Map<String, Integer> refCounts, IndexCommit indexCommit ) throws IOException {
-    log.info("Deleting index files for index commit with generation {} in directory {}", indexCommit.getGeneration(), dir);
-    for (String fileName : indexCommit.getFileNames()) {
-      try {
-        // Ensure that a file being deleted is not referred by some other commit.
-        int ref = refCounts.getOrDefault(fileName, 0);
-        log.debug("Reference count for file {} is {}", fileName, ref);
-        if (ref == 0) {
-          dir.deleteFile(fileName);
-        }
-      } catch (IOException e) {
-        log.warn("Unable to delete file {} in directory {} due to exception {}", fileName, dir, e.getMessage());
-      }
+  private static void deleteSnapshotIndexFiles(SolrCore core, Directory dir, IndexDeletionPolicy delPolicy) throws IOException {
+    IndexWriterConfig conf = core.getSolrConfig().indexConfig.toIndexWriterConfig(core);
+    conf.setOpenMode(OpenMode.APPEND);
+    conf.setMergePolicy(NoMergePolicy.INSTANCE);//Don't want to merge any commits here!
+    conf.setIndexDeletionPolicy(delPolicy);
+    conf.setCodec(core.getCodec());
+
+    try (SolrIndexWriter iw = new SolrIndexWriter("SolrSnapshotCleaner", dir, conf)) {
+      // Do nothing. The only purpose of opening index writer is to invoke the Lucene IndexDeletionPolicy#onInit
+      // method so that we can cleanup the files associated with specified index commit.
+      // Note the index writer creates a new commit during the close() operation (which is harmless).
     }
   }
 }

solr/core/src/java/org/apache/solr/handler/IndexFetcher.java

@@ -82,9 +82,6 @@
 import org.apache.solr.core.DirectoryFactory.DirContext;
 import org.apache.solr.core.IndexDeletionPolicyWrapper;
 import org.apache.solr.core.SolrCore;
-import org.apache.solr.core.snapshots.SolrSnapshotManager;
-import org.apache.solr.core.snapshots.SolrSnapshotMetaDataManager;
-import org.apache.solr.core.snapshots.SolrSnapshotMetaDataManager.SnapshotMetaData;
 import org.apache.solr.handler.ReplicationHandler.*;
 import org.apache.solr.request.LocalSolrQueryRequest;
 import org.apache.solr.request.SolrQueryRequest;
@@ -478,17 +475,8 @@ boolean fetchLatestIndex(boolean forceReplication, boolean forceCoreReload) thro
                 // may be closed
                 if (indexDir != null) {
                   solrCore.getDirectoryFactory().doneWithDirectory(indexDir);
-
-                  SolrSnapshotMetaDataManager snapshotsMgr = solrCore.getSnapshotMetaDataManager();
-                  Collection<SnapshotMetaData> snapshots = snapshotsMgr.listSnapshotsInIndexDir(indexDirPath);
-
-                  // Delete the old index directory only if no snapshot exists in that directory.
-                  if(snapshots.isEmpty()) {
-                    LOG.info("removing old index directory " + indexDir);
-                    solrCore.getDirectoryFactory().remove(indexDir);
-                  } else {
-                    SolrSnapshotManager.deleteNonSnapshotIndexFiles(indexDir, snapshots);
-                  }
+                  // Cleanup all index files not associated with any *named* snapshot.
+                  solrCore.deleteNonSnapshotIndexFiles(indexDirPath);
                 }
               }
 

solr/core/src/java/org/apache/solr/handler/RestoreCore.java

@@ -19,7 +19,6 @@
 import java.lang.invoke.MethodHandles;
 import java.net.URI;
 import java.text.SimpleDateFormat;
-import java.util.Collection;
 import java.util.Date;
 import java.util.Locale;
 import java.util.concurrent.Callable;
@@ -33,9 +32,6 @@
 import org.apache.solr.core.DirectoryFactory;
 import org.apache.solr.core.SolrCore;
 import org.apache.solr.core.backup.repository.BackupRepository;
-import org.apache.solr.core.snapshots.SolrSnapshotManager;
-import org.apache.solr.core.snapshots.SolrSnapshotMetaDataManager;
-import org.apache.solr.core.snapshots.SolrSnapshotMetaDataManager.SnapshotMetaData;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -135,16 +131,8 @@ public boolean doRestore() throws Exception {
       }
       if (success) {
         core.getDirectoryFactory().doneWithDirectory(indexDir);
-
-        SolrSnapshotMetaDataManager snapshotsMgr = core.getSnapshotMetaDataManager();
-        Collection<SnapshotMetaData> snapshots = snapshotsMgr.listSnapshotsInIndexDir(indexDirPath);
-
-        // Delete the old index directory only if no snapshot exists in that directory.
-        if (snapshots.isEmpty()) {
-          core.getDirectoryFactory().remove(indexDir);
-        } else {
-          SolrSnapshotManager.deleteNonSnapshotIndexFiles(indexDir, snapshots);
-        }
+        // Cleanup all index files not associated with any *named* snapshot.
+        core.deleteNonSnapshotIndexFiles(indexDirPath);
       }
 
       return true;