diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index f4335c5e5dbd..725662e29b52 100644 --- a/lucene/CHANGES.txt +++ b/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 and clean up persistence logic. (Greg Miller) + Optimizations --------------------- * GITHUB#16394: Add DocIdSetIterator#intoArray, which DisjunctionDISIApproximation implements by diff --git a/lucene/core/src/java/org/apache/lucene/index/PersistentSnapshotDeletionPolicy.java b/lucene/core/src/java/org/apache/lucene/index/PersistentSnapshotDeletionPolicy.java index d3fa95c16232..6633a3462f7b 100644 --- a/lucene/core/src/java/org/apache/lucene/index/PersistentSnapshotDeletionPolicy.java +++ b/lucene/core/src/java/org/apache/lucene/index/PersistentSnapshotDeletionPolicy.java @@ -19,9 +19,7 @@ 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; @@ -154,8 +152,23 @@ 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 { @@ -195,7 +208,7 @@ 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 { @@ -203,63 +216,43 @@ public String getLastSaveFile() { } } - /** - * 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 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 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; } } diff --git a/lucene/core/src/java/org/apache/lucene/index/SnapshotDeletionPolicy.java b/lucene/core/src/java/org/apache/lucene/index/SnapshotDeletionPolicy.java index 19f2c0d110fc..332a19bff319 100644 --- a/lucene/core/src/java/org/apache/lucene/index/SnapshotDeletionPolicy.java +++ b/lucene/core/src/java/org/apache/lucene/index/SnapshotDeletionPolicy.java @@ -63,20 +63,21 @@ public SnapshotDeletionPolicy(IndexDeletionPolicy primary) { @Override public synchronized void onCommit(List commits) throws IOException { primary.onCommit(wrapCommits(commits)); - lastCommit = commits.get(commits.size() - 1); + lastCommit = commits.getLast(); } @Override public synchronized void onInit(List 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,8 +92,8 @@ 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()"); } @@ -100,29 +101,22 @@ protected void releaseGen(long gen) throws IOException { if (refCount == null) { 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.merge(gen, 1, Integer::sum); + if (refCount == 1) { + indexCommits.put(gen, ic); } - refCounts.put(gen, refCountInt + 1); } /** @@ -140,7 +134,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()"); } @@ -162,8 +156,8 @@ public synchronized List getSnapshots() { /** Returns the total number of snapshots currently held. */ public synchronized int getSnapshotCount() { int total = 0; - for (Integer refCount : refCounts.values()) { - total += refCount.intValue(); + for (int refCount : refCounts.values()) { + total += refCount; } return total; @@ -190,10 +184,10 @@ private List wrapCommits(List 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 +201,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(); } }