Revert "Ensure fully deleted segments are accounted for correctly (elastic#33757)"

This reverts commit 48a5b45.

Author: s1monw

x-pack/plugin/core/src/main/java/org/elasticsearch/snapshots/SourceOnlySnapshot.java

@@ -81,25 +81,15 @@ public synchronized List<String> syncSnapshot(IndexCommit commit) throws IOExcep
  try (Lock writeLock = targetDirectory.obtainLock(IndexWriter.WRITE_LOCK_NAME);
  StandardDirectoryReader reader = (StandardDirectoryReader) DirectoryReader.open(commit)) {
  SegmentInfos segmentInfos = reader.getSegmentInfos();
+ DirectoryReader wrapper = wrapReader(reader);
  List<SegmentCommitInfo> newInfos = new ArrayList<>();
- for (LeafReaderContext ctx : reader.leaves()) {
+ for (LeafReaderContext ctx : wrapper.leaves()) {
+ SegmentCommitInfo info = segmentInfos.info(ctx.ord);
  LeafReader leafReader = ctx.reader();
- SegmentCommitInfo info = reader.getSegmentInfos().info(ctx.ord);
- assert info.info.equals(Lucene.segmentReader(ctx.reader()).getSegmentInfo().info);
- /* We could do this totally different without wrapping this dummy directory reader if FilterCodecReader would have a
- * getDelegate method. This is fixed in LUCENE-8502 but we need to wait for it to come in 7.5.1 or 7.6.
- * The reason here is that the ctx.ord is not guaranteed to be equivalent to the SegmentCommitInfo ord in the SegmentInfo
- * object since we might drop fully deleted segments. if that happens we are using the wrong reader for the SI and
- * might almost certainly expose deleted documents.
- */
- DirectoryReader wrappedReader = wrapReader(new DummyDirectoryReader(reader.directory(), leafReader));
- if (wrappedReader.leaves().isEmpty() == false) {
- leafReader = wrappedReader.leaves().get(0).reader();
- LiveDocs liveDocs = getLiveDocs(leafReader);
- if (leafReader.numDocs() != 0) { // fully deleted segments don't need to be processed
- SegmentCommitInfo newInfo = syncSegment(info, liveDocs, leafReader.getFieldInfos(), existingSegments, createdFiles);
- newInfos.add(newInfo);
- }
+ LiveDocs liveDocs = getLiveDocs(leafReader);
+ if (leafReader.numDocs() != 0) { // fully deleted segments don't need to be processed
+ SegmentCommitInfo newInfo = syncSegment(info, liveDocs, leafReader.getFieldInfos(), existingSegments, createdFiles);
+ newInfos.add(newInfo);
  }
  }
  segmentInfos.clear();
@@ -267,51 +257,4 @@ private static class LiveDocs {
  this.bits = bits;
  }
  }
-
- private static class DummyDirectoryReader extends DirectoryReader {
-
- protected DummyDirectoryReader(Directory directory, LeafReader... segmentReaders) throws IOException {
- super(directory, segmentReaders);
- }
-
- @Override
- protected DirectoryReader doOpenIfChanged() throws IOException {
- return null;
- }
-
- @Override
- protected DirectoryReader doOpenIfChanged(IndexCommit commit) throws IOException {
- return null;
- }
-
- @Override
- protected DirectoryReader doOpenIfChanged(IndexWriter writer, boolean applyAllDeletes) throws IOException {
- return null;
- }
-
- @Override
- public long getVersion() {
- return 0;
- }
-
- @Override
- public boolean isCurrent() throws IOException {
- return false;
- }
-
- @Override
- public IndexCommit getIndexCommit() throws IOException {
- return null;
- }
-
- @Override
- protected void doClose() throws IOException {
-
- }
-
- @Override
- public CacheHelper getReaderCacheHelper() {
- return null;
- }
- }
 }

x-pack/plugin/core/src/main/java/org/elasticsearch/snapshots/SourceOnlySnapshotRepository.java

@@ -130,7 +130,7 @@ protected void closeInternal() {
  SourceOnlySnapshot snapshot = new SourceOnlySnapshot(tempStore.directory(), querySupplier);
  snapshot.syncSnapshot(snapshotIndexCommit);
  // we will use the lucene doc ID as the seq ID so we set the local checkpoint to maxDoc with a new index UUID
- SegmentInfos segmentInfos = tempStore.readLastCommittedSegmentsInfo();
+ SegmentInfos segmentInfos = store.readLastCommittedSegmentsInfo();
  tempStore.bootstrapNewHistory(segmentInfos.totalMaxDoc());
  store.incRef();
  try (DirectoryReader reader = DirectoryReader.open(tempStore.directory())) {

x-pack/plugin/core/src/test/java/org/elasticsearch/snapshots/SourceOnlySnapshotIT.java

@@ -49,7 +49,7 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.ExecutionException;
-import java.util.function.BiConsumer;
+import java.util.function.Consumer;
 
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
@@ -97,10 +97,7 @@ public void testSnapshotAndRestore() throws Exception {
  boolean requireRouting = randomBoolean();
  boolean useNested = randomBoolean();
  IndexRequestBuilder[] builders = snashotAndRestore(sourceIdx, 1, true, requireRouting, useNested);
- IndicesStatsResponse indicesStatsResponse = client().admin().indices().prepareStats(sourceIdx).clear().setDocs(true).get();
- long deleted = indicesStatsResponse.getTotal().docs.getDeleted();
- boolean sourceHadDeletions = deleted > 0; // we use indexRandom which might create holes ie. deleted docs
- assertHits(sourceIdx, builders.length, sourceHadDeletions);
+ assertHits(sourceIdx, builders.length);
  assertMappings(sourceIdx, requireRouting, useNested);
  SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, () -> {
  client().prepareSearch(sourceIdx).setQuery(QueryBuilders.idsQuery()
@@ -119,7 +116,7 @@ public void testSnapshotAndRestore() throws Exception {
  client().admin().indices().prepareUpdateSettings(sourceIdx)
  .setSettings(Settings.builder().put("index.number_of_replicas", 1)).get();
  ensureGreen(sourceIdx);
- assertHits(sourceIdx, builders.length, sourceHadDeletions);
+ assertHits(sourceIdx, builders.length);
  }
 
  public void testSnapshotAndRestoreWithNested() throws Exception {
@@ -128,7 +125,7 @@ public void testSnapshotAndRestoreWithNested() throws Exception {
  IndexRequestBuilder[] builders = snashotAndRestore(sourceIdx, 1, true, requireRouting, true);
  IndicesStatsResponse indicesStatsResponse = client().admin().indices().prepareStats().clear().setDocs(true).get();
  assertThat(indicesStatsResponse.getTotal().docs.getDeleted(), Matchers.greaterThan(0L));
- assertHits(sourceIdx, builders.length, true);
+ assertHits(sourceIdx, builders.length);
  assertMappings(sourceIdx, requireRouting, true);
  SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, () ->
  client().prepareSearch(sourceIdx).setQuery(QueryBuilders.idsQuery().addIds("" + randomIntBetween(0, builders.length))).get());
@@ -144,7 +141,7 @@ public void testSnapshotAndRestoreWithNested() throws Exception {
  client().admin().indices().prepareUpdateSettings(sourceIdx).setSettings(Settings.builder().put("index.number_of_replicas", 1))
  .get();
  ensureGreen(sourceIdx);
- assertHits(sourceIdx, builders.length, true);
+ assertHits(sourceIdx, builders.length);
  }
 
  private void assertMappings(String sourceIdx, boolean requireRouting, boolean useNested) throws IOException {
@@ -168,12 +165,15 @@ private void assertMappings(String sourceIdx, boolean requireRouting, boolean us
  }
  }
 
- private void assertHits(String index, int numDocsExpected, boolean sourceHadDeletions) {
+ private void assertHits(String index, int numDocsExpected) {
  SearchResponse searchResponse = client().prepareSearch(index)
  .addSort(SeqNoFieldMapper.NAME, SortOrder.ASC)
  .setSize(numDocsExpected).get();
- BiConsumer<SearchResponse, Boolean> assertConsumer = (res, allowHoles) -> {
+ Consumer<SearchResponse> assertConsumer = res -> {
  SearchHits hits = res.getHits();
+ IndicesStatsResponse indicesStatsResponse = client().admin().indices().prepareStats().clear().setDocs(true).get();
+ long deleted = indicesStatsResponse.getTotal().docs.getDeleted();
+ boolean allowHoles = deleted > 0; // we use indexRandom which might create holes ie. deleted docs
  long i = 0;
  for (SearchHit hit : hits) {
  String id = hit.getId();
@@ -190,24 +190,18 @@ private void assertHits(String index, int numDocsExpected, boolean sourceHadDele
  assertEquals("r" + id, hit.field("_routing").getValue());
  }
  };
- assertConsumer.accept(searchResponse, sourceHadDeletions);
+ assertConsumer.accept(searchResponse);
  assertEquals(numDocsExpected, searchResponse.getHits().totalHits);
  searchResponse = client().prepareSearch(index)
  .addSort(SeqNoFieldMapper.NAME, SortOrder.ASC)
  .setScroll("1m")
  .slice(new SliceBuilder(SeqNoFieldMapper.NAME, randomIntBetween(0,1), 2))
  .setSize(randomIntBetween(1, 10)).get();
- try {
- do {
- // now do a scroll with a slice
- assertConsumer.accept(searchResponse, true);
- searchResponse = client().prepareSearchScroll(searchResponse.getScrollId()).setScroll(TimeValue.timeValueMinutes(1)).get();
- } while (searchResponse.getHits().getHits().length > 0);
- } finally {
- if (searchResponse.getScrollId() != null) {
- client().prepareClearScroll().addScrollId(searchResponse.getScrollId()).get();
- }
- }
+ do {
+ // now do a scroll with a slice
+ assertConsumer.accept(searchResponse);
+ searchResponse = client().prepareSearchScroll(searchResponse.getScrollId()).setScroll(TimeValue.timeValueMinutes(1)).get();
+ } while (searchResponse.getHits().getHits().length > 0);
 
  }
 

x-pack/plugin/core/src/test/java/org/elasticsearch/snapshots/SourceOnlySnapshotShardTests.java

@@ -162,6 +162,7 @@ private String randomDoc() {
  return "{ \"value\" : \"" + randomAlphaOfLength(10) + "\"}";
  }
 
+ @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/33689")
  public void testRestoreMinmal() throws IOException {
  IndexShard shard = newStartedShard(true);
  int numInitialDocs = randomIntBetween(10, 100);

x-pack/plugin/core/src/test/java/org/elasticsearch/snapshots/SourceOnlySnapshotTests.java

@@ -12,12 +12,10 @@
 import org.apache.lucene.document.StoredField;
 import org.apache.lucene.document.StringField;
 import org.apache.lucene.document.TextField;
-import org.apache.lucene.index.CodecReader;
 import org.apache.lucene.index.DirectoryReader;
 import org.apache.lucene.index.FilterMergePolicy;
 import org.apache.lucene.index.IndexCommit;
 import org.apache.lucene.index.IndexFileNames;
-import org.apache.lucene.index.IndexReader;
 import org.apache.lucene.index.IndexWriter;
 import org.apache.lucene.index.IndexWriterConfig;
 import org.apache.lucene.index.KeepOnlyLastCommitDeletionPolicy;
@@ -36,7 +34,6 @@
 import org.apache.lucene.search.TermQuery;
 import org.apache.lucene.search.TopDocs;
 import org.apache.lucene.store.Directory;
-import org.apache.lucene.util.IOSupplier;
 import org.elasticsearch.common.lucene.Lucene;
 import org.elasticsearch.test.ESTestCase;
 
@@ -245,55 +242,4 @@ public boolean useCompoundFile(SegmentInfos infos, SegmentCommitInfo mergedInfo,
  reader.close();
  }
  }
-
- public void testFullyDeletedSegments() throws IOException {
- try (Directory dir = newDirectory()) {
- SnapshotDeletionPolicy deletionPolicy = new SnapshotDeletionPolicy(new KeepOnlyLastCommitDeletionPolicy());
- IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig()
- .setSoftDeletesField(Lucene.SOFT_DELETES_FIELD)
- .setIndexDeletionPolicy(deletionPolicy).setMergePolicy(new FilterMergePolicy(NoMergePolicy.INSTANCE) {
- @Override
- public boolean useCompoundFile(SegmentInfos infos, SegmentCommitInfo mergedInfo, MergeContext mergeContext) {
- return randomBoolean();
- }
-
- @Override
- public boolean keepFullyDeletedSegment(IOSupplier<CodecReader> readerIOSupplier) throws IOException {
- return true;
- }
- }));
- Document doc = new Document();
- doc.add(new StringField("id", "1", Field.Store.YES));
- doc.add(new TextField("text", "the quick brown fox", Field.Store.NO));
- doc.add(new NumericDocValuesField("rank", 1));
- doc.add(new StoredField("rank", 1));
- doc.add(new StoredField("src", "the quick brown fox"));
- writer.addDocument(doc);
- writer.commit();
- doc = new Document();
- doc.add(new StringField("id", "1", Field.Store.YES));
- doc.add(new TextField("text", "the quick brown fox", Field.Store.NO));
- doc.add(new NumericDocValuesField("rank", 3));
- doc.add(new StoredField("rank", 3));
- doc.add(new StoredField("src", "the quick brown fox"));
- writer.softUpdateDocument(new Term("id", "1"), doc, new NumericDocValuesField(Lucene.SOFT_DELETES_FIELD, 1));
- writer.commit();
- try (Directory targetDir = newDirectory()) {
- IndexCommit snapshot = deletionPolicy.snapshot();
- SourceOnlySnapshot snapshoter = new SourceOnlySnapshot(targetDir);
- snapshoter.syncSnapshot(snapshot);
-
- try (DirectoryReader snapReader = DirectoryReader.open(targetDir)) {
- assertEquals(snapReader.maxDoc(), 1);
- assertEquals(snapReader.numDocs(), 1);
- assertEquals("3", snapReader.document(0).getField("rank").stringValue());
- }
- try (IndexReader writerReader = DirectoryReader.open(writer)) {
- assertEquals(writerReader.maxDoc(), 2);
- assertEquals(writerReader.numDocs(), 1);
- }
- }
- writer.close();
- }
- }
 }