From 1249e6ba5d12bf6cd2e8935a8e6a1fb87e83702a Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 28 Aug 2019 13:44:39 -0400 Subject: [PATCH] Handle no-op document level failures (#46083) Today we assume that document failures can not occur for no-ops. This assumption is bogus, as they can fail for a variety of reasons such as the Lucene index having reached the document limit. Because of this assumption, we were asserting that such a document-level failure would never happen. When this bogus assertion is violated, we fail the node, a catastrophe. Instead, we need to treat this as a fatal engine exception. --- .../index/engine/InternalEngine.java | 10 ++++++-- .../index/engine/InternalEngineTests.java | 25 +++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) 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 b83c0a70178..335e392495c 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java @@ -1514,9 +1514,14 @@ public class InternalEngine extends Engine { : "Noop tombstone document but _tombstone field is not set [" + doc + " ]"; doc.add(softDeletesField); indexWriter.addDocument(doc); - } catch (Exception ex) { + } catch (final Exception ex) { + /* + * Document level failures when adding a no-op are unexpected, we likely hit something fatal such as the Lucene + * index being corrupt, or the Lucene document limit. We have already issued a sequence number here so this is + * fatal, fail the engine. + */ if (ex instanceof AlreadyClosedException == false && indexWriter.getTragicException() == null) { - throw new AssertionError("noop operation should never fail at document level", ex); + failEngine("no-op origin[" + noOp.origin() + "] seq#[" + noOp.seqNo() + "] failed at document level", ex); } throw ex; } @@ -2852,4 +2857,5 @@ public class InternalEngine extends Engine { // remove live entries in the version map refresh("restore_version_map_and_checkpoint_tracker", SearcherScope.INTERNAL, true); } + } 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 24872cc3330..f8d516002da 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java @@ -5972,4 +5972,29 @@ public class InternalEngineTests extends EngineTestCase { equalTo(seqNos)); } } + + public void testNoOpFailure() throws IOException { + engine.close(); + final Settings settings = Settings.builder() + .put(defaultSettings.getSettings()) + .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true).build(); + final IndexSettings indexSettings = IndexSettingsModule.newIndexSettings( + IndexMetaData.builder(defaultSettings.getIndexMetaData()).settings(settings).build()); + try (Store store = createStore(); + Engine engine = createEngine((dir, iwc) -> new IndexWriter(dir, iwc) { + @Override + public long addDocument(Iterable doc) throws IOException { + throw new IllegalArgumentException("fatal"); + } + }, null, null, config(indexSettings, store, createTempDir(), NoMergePolicy.INSTANCE, null))) { + final Engine.NoOp op = new Engine.NoOp(0, 0, PRIMARY, System.currentTimeMillis(), "test"); + final IllegalArgumentException e = expectThrows(IllegalArgumentException. class, () -> engine.noOp(op)); + assertThat(e.getMessage(), equalTo("fatal")); + assertTrue(engine.isClosed.get()); + assertThat(engine.failedEngine.get(), not(nullValue())); + assertThat(engine.failedEngine.get(), instanceOf(IllegalArgumentException.class)); + assertThat(engine.failedEngine.get().getMessage(), equalTo("fatal")); + } + } + }