LUCENE-9980: Do not expose deleted commits (#158)

If we fail to delete files that belong to a commit point, then we will 
expose that deleted commit in the next calls of IndexDeletionPolicy#onCommit.
I think we should never expose those deleted commit points as 
some of their files might have been deleted already.
This commit is contained in:
Nhat Nguyen 2021-05-31 11:03:48 -04:00 committed by GitHub
parent d76dd6454e
commit c46bcf75cc
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 60 additions and 4 deletions

View File

@ -440,10 +440,6 @@ final class IndexFileDeleter implements Closeable {
} }
commitsToDelete.clear(); commitsToDelete.clear();
if (firstThrowable != null) {
throw IOUtils.rethrowAlways(firstThrowable);
}
// Now compact commits to remove deleted ones (preserving the sort): // Now compact commits to remove deleted ones (preserving the sort):
size = commits.size(); size = commits.size();
int readFrom = 0; int readFrom = 0;
@ -463,6 +459,10 @@ final class IndexFileDeleter implements Closeable {
commits.remove(size - 1); commits.remove(size - 1);
size--; size--;
} }
if (firstThrowable != null) {
throw IOUtils.rethrowAlways(firstThrowable);
}
} }
} }
@ -518,6 +518,13 @@ final class IndexFileDeleter implements Closeable {
} }
} }
boolean assertCommitsAreNotDeleted(List<CommitPoint> commits) {
for (CommitPoint commit : commits) {
assert commit.isDeleted() == false : "Commit [" + commit + "] was deleted already";
}
return true;
}
/** /**
* Revisits the {@link IndexDeletionPolicy} by calling its {@link * Revisits the {@link IndexDeletionPolicy} by calling its {@link
* IndexDeletionPolicy#onCommit(List)} again with the known commits. This is useful in cases where * IndexDeletionPolicy#onCommit(List)} again with the known commits. This is useful in cases where
@ -532,6 +539,7 @@ final class IndexFileDeleter implements Closeable {
} }
if (commits.size() > 0) { if (commits.size() > 0) {
assert assertCommitsAreNotDeleted(commits);
policy.onCommit(commits); policy.onCommit(commits);
deleteCommits(); deleteCommits();
} }
@ -578,6 +586,7 @@ final class IndexFileDeleter implements Closeable {
commits.add(new CommitPoint(commitsToDelete, directoryOrig, segmentInfos)); commits.add(new CommitPoint(commitsToDelete, directoryOrig, segmentInfos));
// Tell policy so it can remove commits: // Tell policy so it can remove commits:
assert assertCommitsAreNotDeleted(commits);
policy.onCommit(commits); policy.onCommit(commits);
// Decref files for commits that were deleted by the policy: // Decref files for commits that were deleted by the policy:

View File

@ -575,4 +575,51 @@ public class TestIndexFileDeleter extends LuceneTestCase {
dir.close(); dir.close();
} }
} }
public void testThrowExceptionWhileDeleteCommits() throws Exception {
try (MockDirectoryWrapper dir = newMockDirectory()) {
final AtomicBoolean failOnDeleteCommits = new AtomicBoolean();
dir.failOn(
new MockDirectoryWrapper.Failure() {
@Override
public void eval(MockDirectoryWrapper dir) throws IOException {
if (failOnDeleteCommits.get()) {
if (callStackContains(IndexFileDeleter.class, "deleteCommits")
&& callStackContains(MockDirectoryWrapper.class, "deleteFile")
&& random().nextInt(4) == 1) {
throw new MockDirectoryWrapper.FakeIOException();
}
}
}
});
final SnapshotDeletionPolicy snapshotDeletionPolicy =
new SnapshotDeletionPolicy(new KeepOnlyLastCommitDeletionPolicy());
final IndexWriterConfig config =
newIndexWriterConfig().setIndexDeletionPolicy(snapshotDeletionPolicy);
try (RandomIndexWriter writer = new RandomIndexWriter(random(), dir, config)) {
writer.addDocument(new Document());
writer.commit();
final IndexCommit snapshotCommit = snapshotDeletionPolicy.snapshot();
int commits = TestUtil.nextInt(random(), 1, 3);
for (int i = 0; i < commits; i++) {
writer.addDocument(new Document());
writer.commit();
}
snapshotDeletionPolicy.release(snapshotCommit);
failOnDeleteCommits.set(true);
try {
writer.w.deleteUnusedFiles();
} catch (IOException e) {
assertTrue(e instanceof MockDirectoryWrapper.FakeIOException);
}
failOnDeleteCommits.set(false);
for (int c = 0; c < commits; c++) {
writer.addDocument(new Document());
writer.commit();
}
}
}
}
} }