epotyom commented on code in PR #16482:
URL: https://github.com/apache/lucene/pull/16482#discussion_r3852014273
##########
lucene/core/src/java/org/apache/lucene/index/SnapshotDeletionPolicy.java:
##########
@@ -91,38 +92,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) {
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);
Review Comment:
Thanks for fixing this. Storing `lastCommit` instead of `ic` cause issues
when
I tried extending this class before.
##########
lucene/core/src/java/org/apache/lucene/index/PersistentSnapshotDeletionPolicy.java:
##########
@@ -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 {
Review Comment:
Trying to understand what changes in this method; given that normally we
only have one snapshot file, I think the only case where behavior really
differs is when `persist` fails to write the new file and its cleanup
(`IOUtils.deleteFilesSuppressingExceptions`) also fails, leaving a corrupt
newest file behind.
Say gen 4 is the last good file and gen 5 is the corrupt one. Before this
change we would read gen 4 into `refCounts`, then overwrite it with whatever
came back from gen 5 (empty or partial), set `genLoaded = 5`, and then delete
gen 4 as a stale file without throwing any exception.
With this change we throw instead, and gen 4 survives.
The question is, do we want to throw here? It is better than silently
dropping snapshots, but it does mean the writer can't be opened until someone
manually deletes the corrupt file. The alternative is to keep the ref-counts
from the newest file we can actually read, wdyt?
We might also want to add a unit test for this scenario.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
