Require soft-deletes when access changes snapshot (#36446)

Today we do not enforce soft-deletes when accessing the Lucene changes snapshot. This might lead to internal errors because we assume soft-deletes are enabled in that code path.
elastic · Dec 11, 2018 · 084e06e · 084e06e
1 parent 8b82170
commit 084e06e
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 5 deletions.
diff --git a/server/src/main/java/org/elasticsearch/index/engine/Engine.java b/server/src/main/java/org/elasticsearch/index/engine/Engine.java
@@ -722,7 +722,8 @@ public enum SearcherScope {
     public abstract Closeable acquireRetentionLockForPeerRecovery();
 
     /**
-     * Creates a new history snapshot from Lucene for reading operations whose seqno in the requesting seqno range (both inclusive)
+     * Creates a new history snapshot from Lucene for reading operations whose seqno in the requesting seqno range (both inclusive).
+     * This feature requires soft-deletes enabled. If soft-deletes are disabled, this method will throw an {@link IllegalStateException}.
      */
     public abstract Translog.Snapshot newChangesSnapshot(String source, MapperService mapperService,
                                                          long fromSeqNo, long toSeqNo, boolean requiredFullRange) throws IOException;

diff --git a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java
@@ -2481,7 +2481,9 @@ long getNumDocUpdates() {
     @Override
     public Translog.Snapshot newChangesSnapshot(String source, MapperService mapperService,
                                                 long fromSeqNo, long toSeqNo, boolean requiredFullRange) throws IOException {
-        // TODO: Should we defer the refresh until we really need it?
+        if (softDeleteEnabled == false) {
+            throw new IllegalStateException("accessing changes snapshot requires soft-deletes enabled");
+        }
         ensureOpen();
         refreshIfNeeded(source, toSeqNo);
         Searcher searcher = acquireSearcher(source, SearcherScope.INTERNAL);

diff --git a/server/src/main/java/org/elasticsearch/index/engine/ReadOnlyEngine.java b/server/src/main/java/org/elasticsearch/index/engine/ReadOnlyEngine.java
@@ -243,6 +243,9 @@ public Closeable acquireRetentionLockForPeerRecovery() {
     @Override
     public Translog.Snapshot newChangesSnapshot(String source, MapperService mapperService, long fromSeqNo, long toSeqNo,
                                                 boolean requiredFullRange) throws IOException {
+        if (engineConfig.getIndexSettings().isSoftDeleteEnabled() == false) {
+            throw new IllegalStateException("accessing changes snapshot requires soft-deletes enabled");
+        }
         return readHistoryOperations(source, mapperService, fromSeqNo);
     }
 

diff --git a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java
@@ -3411,8 +3411,10 @@ public void testDoubleDeliveryReplica() throws IOException {
             TopDocs topDocs = searcher.searcher().search(new MatchAllDocsQuery(), 10);
             assertEquals(1, topDocs.totalHits.value);
         }
-        List<Translog.Operation> ops = readAllOperationsInLucene(engine, createMapperService("test"));
-        assertThat(ops.stream().map(o -> o.seqNo()).collect(Collectors.toList()), hasItem(20L));
+        if (engine.engineConfig.getIndexSettings().isSoftDeleteEnabled()) {
+            List<Translog.Operation> ops = readAllOperationsInLucene(engine, createMapperService("test"));
+            assertThat(ops.stream().map(o -> o.seqNo()).collect(Collectors.toList()), hasItem(20L));
+        }
     }
 
     public void testRetryWithAutogeneratedIdWorksAndNoDuplicateDocs() throws IOException {
@@ -5292,8 +5294,11 @@ public void testLuceneSnapshotRefreshesOnlyOnce() throws Exception {
         final MapperService mapperService = createMapperService("test");
         final long maxSeqNo = randomLongBetween(10, 50);
         final AtomicLong refreshCounter = new AtomicLong();
+        final IndexSettings indexSettings = IndexSettingsModule.newIndexSettings(
+            IndexMetaData.builder(defaultSettings.getIndexMetaData()).settings(Settings.builder().
+                put(defaultSettings.getSettings()).put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true)).build());
         try (Store store = createStore();
-             InternalEngine engine = createEngine(config(defaultSettings, store, createTempDir(), newMergePolicy(),
+             InternalEngine engine = createEngine(config(indexSettings, store, createTempDir(), newMergePolicy(),
                  null,
                  new ReferenceManager.RefreshListener() {
                      @Override
@@ -5481,6 +5486,19 @@ public void testOpenSoftDeletesIndexWithSoftDeletesDisabled() throws Exception {
         }
     }
 
+    public void testRequireSoftDeletesWhenAccessingChangesSnapshot() throws Exception {
+        try (Store store = createStore()) {
+            final IndexSettings indexSettings = IndexSettingsModule.newIndexSettings(
+                IndexMetaData.builder(defaultSettings.getIndexMetaData()).settings(Settings.builder().
+                    put(defaultSettings.getSettings()).put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), false)).build());
+            try (InternalEngine engine = createEngine(config(indexSettings, store, createTempDir(), newMergePolicy(), null))) {
+                IllegalStateException error = expectThrows(IllegalStateException.class,
+                    () -> engine.newChangesSnapshot("test", createMapperService("test"), 0, randomNonNegativeLong(), randomBoolean()));
+                assertThat(error.getMessage(), equalTo("accessing changes snapshot requires soft-deletes enabled"));
+            }
+        }
+    }
+
     static void trimUnsafeCommits(EngineConfig config) throws IOException {
         final Store store = config.getStore();
         final TranslogConfig translogConfig = config.getTranslogConfig();