From 084e06e481bc14f0ec1a71cc0444b1e05a1bbe20 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Tue, 11 Dec 2018 11:18:10 -0500 Subject: [PATCH] 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. --- .../elasticsearch/index/engine/Engine.java | 3 ++- .../index/engine/InternalEngine.java | 4 +++- .../index/engine/ReadOnlyEngine.java | 3 +++ .../index/engine/InternalEngineTests.java | 24 ++++++++++++++++--- 4 files changed, 29 insertions(+), 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 index ec3afe9e080bf..121350df8ca38 100644 --- 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 index 4ac63dacca297..8179195206fc6 100644 --- 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 index 6626d4758f4d9..c926a5a47191e 100644 --- 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 index 0bc0504641874..c476eb8d466dd 100644 --- 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 ops = readAllOperationsInLucene(engine, createMapperService("test")); - assertThat(ops.stream().map(o -> o.seqNo()).collect(Collectors.toList()), hasItem(20L)); + if (engine.engineConfig.getIndexSettings().isSoftDeleteEnabled()) { + List 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();