Modernize SnapshotDeletionPolicy and PersistentSnapshotDeletionPolicy

lucene/CHANGES.txt

@@ -352,6 +352,9 @@ Improvements
   The new code allows to add a hard limit for nesting (default is 1024) and no longer
   throws virtual machine errors.  (Uwe Schindler, Seth Kraft)
 
+* GITHUB#16482: Modernize SnapshotDeletionPolicy and PersistentSnapshotDeletionPolicy through minor
+  improvements to data structure choice and persistence logic. (Greg Miller)
+
 Optimizations
 ---------------------
 * GITHUB#16394: Add DocIdSetIterator#intoArray, which DisjunctionDISIApproximation implements by

lucene/MIGRATE.md

@@ -219,6 +219,13 @@ Query query = new AutomatonQuery(new Term("myfield", pattern), dfa);
 
 Corresponding methods and parameters have been renamed accordingly.
 
+## Migration from Lucene 10.5 to Lucene 10.6
+
+### `SnapshotDeletionPolicy#[refCounts|indexCommits]` type change
+
+These two protected fields changed from java.util collections to hppc collections. Subclasses will
+need to adapt if they access these fields directly.
+
 ## Migration from Lucene 10.4 to Lucene 10.5
 
 ### `[Byte|Float]VectorSimilarityQuery` now performs adaptive HNSW graph traversal

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

@@ -19,12 +19,10 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
-import java.util.Map.Entry;
 import org.apache.lucene.codecs.CodecUtil;
 import org.apache.lucene.index.IndexWriterConfig.OpenMode;
+import org.apache.lucene.internal.hppc.LongIntHashMap;
 import org.apache.lucene.store.Directory;
 import org.apache.lucene.store.IOContext;
 import org.apache.lucene.store.IndexInput;
@@ -154,18 +152,33 @@ public synchronized void release(IndexCommit commit) throws IOException {
    * @see SnapshotDeletionPolicy#release
    */
   public synchronized void release(long gen) throws IOException {
+    IndexCommit ic = super.getIndexCommit(gen);
     super.releaseGen(gen);
-    persist();
+    try {
+      persist();
+    } catch (Throwable t) {
+      try {
+        // If we get in a state where ic is null it indicates we had ref-counts for a gen on disk
+        // that we didn't see corresponding commits for in onInit. In that situation, we don't
+        // re-increment ref-counts on error.
+        if (ic != null) {
+          incRef(ic);
+        }
+      } catch (Exception e) {
+        t.addSuppressed(e);
+      }
+      throw t;
+    }
   }
 
   private synchronized void persist() throws IOException {
     String fileName = SNAPSHOTS_PREFIX + nextWriteGen;
     try (IndexOutput out = dir.createOutput(fileName, IOContext.DEFAULT)) {
       CodecUtil.writeHeader(out, CODEC_NAME, VERSION_CURRENT);
       out.writeVInt(refCounts.size());
-      for (Entry<Long, Integer> ent : refCounts.entrySet()) {
-        out.writeVLong(ent.getKey());
-        out.writeVInt(ent.getValue());
+      for (LongIntHashMap.LongIntCursor ent : refCounts) {
+        out.writeVLong(ent.key);
+        out.writeVInt(ent.value);
       }
     } catch (Throwable t) {
       IOUtils.deleteFilesSuppressingExceptions(t, dir, fileName);
@@ -195,71 +208,51 @@ private synchronized void clearPriorSnapshots() throws IOException {
    * Returns the file name the snapshots are currently saved to, or null if no snapshots have been
    * saved.
    */
-  public String getLastSaveFile() {
+  public synchronized String getLastSaveFile() {
     if (nextWriteGen == 0) {
       return null;
     } else {
       return SNAPSHOTS_PREFIX + (nextWriteGen - 1);
     }
   }
 
-  /**
-   * Reads the snapshots information from the given {@link Directory}. This method can be used if
-   * the snapshots information is needed, however you cannot instantiate the deletion policy
-   * (because e.g., some other process keeps a lock on the snapshots directory).
-   */
   private synchronized void loadPriorSnapshots() throws IOException {
     long genLoaded = -1;
-    IOException ioe = null;
     List<String> snapshotFiles = new ArrayList<>();
     for (String file : dir.listAll()) {
       if (file.startsWith(SNAPSHOTS_PREFIX)) {
+        snapshotFiles.add(file);
         long gen = Long.parseLong(file.substring(SNAPSHOTS_PREFIX.length()));
-        if (genLoaded == -1 || gen > genLoaded) {
-          snapshotFiles.add(file);
-          Map<Long, Integer> m = new HashMap<>();
-          IndexInput in = dir.openInput(file, IOContext.DEFAULT);
-          try {
-            CodecUtil.checkHeader(in, CODEC_NAME, VERSION_START, VERSION_START);
-            int count = in.readVInt();
-            for (int i = 0; i < count; i++) {
-              long commitGen = in.readVLong();
-              int refCount = in.readVInt();
-              m.put(commitGen, refCount);
-            }
-          } catch (IOException ioe2) {
-            // Save first exception & throw in the end
-            if (ioe == null) {
-              ioe = ioe2;
-            }
-          } finally {
-            in.close();
-          }
-
+        if (gen > genLoaded) {
           genLoaded = gen;
-          refCounts.clear();
-          refCounts.putAll(m);
         }
       }
     }
 
     if (genLoaded == -1) {
-      // Nothing was loaded...
-      if (ioe != null) {
-        // ... not for lack of trying:
-        throw ioe;
+      return;
+    }
+
+    // Load only the latest snapshot file
+    String latestFile = SNAPSHOTS_PREFIX + genLoaded;
+    refCounts.clear();
+    try (IndexInput in = dir.openInput(latestFile, IOContext.DEFAULT)) {
+      CodecUtil.checkHeader(in, CODEC_NAME, VERSION_START, VERSION_START);
+      int count = in.readVInt();
+      for (int i = 0; i < count; i++) {
+        long commitGen = in.readVLong();
+        int refCount = in.readVInt();
+        refCounts.put(commitGen, refCount);
       }
-    } else {
-      if (snapshotFiles.size() > 1) {
-        // Remove any broken / old snapshot files:
-        String curFileName = SNAPSHOTS_PREFIX + genLoaded;
-        for (String file : snapshotFiles) {
-          if (!curFileName.equals(file)) {
-            IOUtils.deleteFilesIgnoringExceptions(dir, file);
-          }
-        }
+    }
+
+    // Clean up old snapshot files
+    for (String file : snapshotFiles) {
+      if (latestFile.equals(file) == false) {
+        IOUtils.deleteFilesIgnoringExceptions(dir, file);
       }
-      nextWriteGen = 1 + genLoaded;
     }
+
+    nextWriteGen = 1 + genLoaded;
   }
 }

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

@@ -19,9 +19,12 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import org.apache.lucene.internal.hppc.IntCursor;
+import org.apache.lucene.internal.hppc.LongIntHashMap;
+import org.apache.lucene.internal.hppc.LongObjectHashMap;
+import org.apache.lucene.internal.hppc.ObjectCursor;
 import org.apache.lucene.store.Directory;
 
 /**
@@ -41,10 +44,10 @@
 public class SnapshotDeletionPolicy extends IndexDeletionPolicy {
 
   /** Records how many snapshots are held against each commit generation */
-  protected final Map<Long, Integer> refCounts = new HashMap<>();
+  protected final LongIntHashMap refCounts = new LongIntHashMap();
 
   /** Used to map gen to IndexCommit. */
-  protected final Map<Long, IndexCommit> indexCommits = new HashMap<>();
+  protected final LongObjectHashMap<IndexCommit> indexCommits = new LongObjectHashMap<>();
 
   /** Wrapped {@link IndexDeletionPolicy} */
   private final IndexDeletionPolicy primary;
@@ -63,20 +66,21 @@ public SnapshotDeletionPolicy(IndexDeletionPolicy primary) {
   @Override
   public synchronized void onCommit(List<? extends IndexCommit> commits) throws IOException {
     primary.onCommit(wrapCommits(commits));
-    lastCommit = commits.get(commits.size() - 1);
+    lastCommit = commits.getLast();
   }
 
   @Override
   public synchronized void onInit(List<? extends IndexCommit> commits) throws IOException {
     initCalled = true;
     primary.onInit(wrapCommits(commits));
-    for (IndexCommit commit : commits) {
-      if (refCounts.containsKey(commit.getGeneration())) {
-        indexCommits.put(commit.getGeneration(), commit);
+    if (commits.isEmpty() == false) {
+      for (IndexCommit commit : commits) {
+        long gen = commit.getGeneration();
+        if (refCounts.containsKey(gen)) {
+          indexCommits.put(gen, commit);
+        }
       }
-    }
-    if (!commits.isEmpty()) {
-      lastCommit = commits.get(commits.size() - 1);
+      lastCommit = commits.getLast();
     }
   }
 
@@ -91,38 +95,31 @@ public synchronized void release(IndexCommit commit) throws IOException {
   }
 
   /** Release a snapshot by generation. */
-  protected void releaseGen(long gen) throws IOException {
-    if (!initCalled) {
+  protected synchronized void releaseGen(long gen) {
+    if (initCalled == false) {
       throw new IllegalStateException(
           "this instance is not being used by IndexWriter; be sure to use the instance returned from writer.getConfig().getIndexDeletionPolicy()");
     }
-    Integer refCount = refCounts.get(gen);
-    if (refCount == null) {
+    int refCount = refCounts.getOrDefault(gen, 0);
+    if (refCount == 0) {
       throw new IllegalArgumentException("commit gen=" + gen + " is not currently snapshotted");
     }
-    int refCountInt = refCount.intValue();
-    assert refCountInt > 0;
-    refCountInt--;
-    if (refCountInt == 0) {
+    assert refCount > 0;
+    if (refCount == 1) {
       refCounts.remove(gen);
       indexCommits.remove(gen);
     } else {
-      refCounts.put(gen, refCountInt);
+      refCounts.put(gen, refCount - 1);
     }
   }
 
   /** Increments the refCount for this {@link IndexCommit}. */
   protected synchronized void incRef(IndexCommit ic) {
     long gen = ic.getGeneration();
-    Integer refCount = refCounts.get(gen);
-    int refCountInt;
-    if (refCount == null) {
-      indexCommits.put(gen, lastCommit);
-      refCountInt = 0;
-    } else {
-      refCountInt = refCount.intValue();
+    int refCount = refCounts.putOrAdd(gen, 1, 1);
+    if (refCount == 1) {
+      indexCommits.put(gen, ic);
     }
-    refCounts.put(gen, refCountInt + 1);
   }
 
   /**
@@ -140,7 +137,7 @@ protected synchronized void incRef(IndexCommit ic) {
    * @return the {@link IndexCommit} that was snapshotted.
    */
   public synchronized IndexCommit snapshot() throws IOException {
-    if (!initCalled) {
+    if (initCalled == false) {
       throw new IllegalStateException(
           "this instance is not being used by IndexWriter; be sure to use the instance returned from writer.getConfig().getIndexDeletionPolicy()");
     }
@@ -156,14 +153,19 @@ public synchronized IndexCommit snapshot() throws IOException {
 
   /** Returns all IndexCommits held by at least one snapshot. */
   public synchronized List<IndexCommit> getSnapshots() {
-    return new ArrayList<>(indexCommits.values());
+    ArrayList<IndexCommit> result = new ArrayList<>(indexCommits.size());
+    for (ObjectCursor<IndexCommit> cursor : indexCommits.values()) {
+      result.add(cursor.value);
+    }
+
+    return result;
   }
 
   /** Returns the total number of snapshots currently held. */
   public synchronized int getSnapshotCount() {
     int total = 0;
-    for (Integer refCount : refCounts.values()) {
-      total += refCount.intValue();
+    for (IntCursor cursor : refCounts.values()) {
+      total += cursor.value;
     }
 
     return total;
@@ -190,10 +192,10 @@ private List<IndexCommit> wrapCommits(List<? extends IndexCommit> commits) {
   private class SnapshotCommitPoint extends IndexCommit {
 
     /** The {@link IndexCommit} we are preventing from deletion. */
-    protected IndexCommit cp;
+    private final IndexCommit cp;
 
     /** Creates a {@code SnapshotCommitPoint} wrapping the provided {@link IndexCommit}. */
-    protected SnapshotCommitPoint(IndexCommit cp) {
+    SnapshotCommitPoint(IndexCommit cp) {
       this.cp = cp;
     }
 
@@ -207,7 +209,7 @@ public void delete() {
       synchronized (SnapshotDeletionPolicy.this) {
         // Suppress the delete request if this commit point is
         // currently snapshotted.
-        if (!refCounts.containsKey(cp.getGeneration())) {
+        if (refCounts.containsKey(cp.getGeneration()) == false) {
           cp.delete();
         }
       }