From ac74e5feee691814359df6d4720e0848a13c434f Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Wed, 18 Jul 2018 09:28:09 +0200 Subject: [PATCH] drop `index.shard.check_on_startup: fix` Closes #31389 --- .../elasticsearch/index/IndexSettings.java | 3 +- .../elasticsearch/index/shard/IndexShard.java | 16 +---- .../org/elasticsearch/index/store/Store.java | 12 ---- .../MetaDataIndexTemplateServiceTests.java | 2 +- .../index/shard/IndexShardTests.java | 61 +++++++++++++++- .../index/shard/IndexShardTestCase.java | 71 +++++++++++++++++-- 6 files changed, 131 insertions(+), 34 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/IndexSettings.java b/server/src/main/java/org/elasticsearch/index/IndexSettings.java index 08cacee6ae0d3..46d7836097edf 100644 --- a/server/src/main/java/org/elasticsearch/index/IndexSettings.java +++ b/server/src/main/java/org/elasticsearch/index/IndexSettings.java @@ -74,11 +74,10 @@ public final class IndexSettings { switch(s) { case "false": case "true": - case "fix": case "checksum": return s; default: - throw new IllegalArgumentException("unknown value for [index.shard.check_on_startup] must be one of [true, false, fix, checksum] but was: " + s); + throw new IllegalArgumentException("unknown value for [index.shard.check_on_startup] must be one of [true, false, checksum] but was: " + s); } }, Property.IndexScope); diff --git a/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java b/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java index fc08438a7d9c5..4e0906facacde 100644 --- a/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java +++ b/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java @@ -1297,7 +1297,7 @@ private void innerOpenEngineAndTranslog() throws IOException { } recoveryState.setStage(RecoveryState.Stage.VERIFY_INDEX); // also check here, before we apply the translog - if (Booleans.isTrue(checkIndexOnStartup)) { + if (Booleans.isTrue(checkIndexOnStartup) || "checksum".equals(checkIndexOnStartup)) { try { checkIndex(); } catch (IOException ex) { @@ -1932,18 +1932,8 @@ private void doCheckIndex() throws IOException { return; } logger.warn("check index [failure]\n{}", os.bytes().utf8ToString()); - if ("fix".equals(checkIndexOnStartup)) { - if (logger.isDebugEnabled()) { - logger.debug("fixing index, writing new segments file ..."); - } - store.exorciseIndex(status); - if (logger.isDebugEnabled()) { - logger.debug("index fixed, wrote new segments file \"{}\"", status.segmentsFileName); - } - } else { - // only throw a failure if we are not going to fix the index - throw new IllegalStateException("index check failure but can't fix it"); - } + // only throw a failure if we are not going to fix the index + throw new IllegalStateException("index check failure but can't fix it"); } } diff --git a/server/src/main/java/org/elasticsearch/index/store/Store.java b/server/src/main/java/org/elasticsearch/index/store/Store.java index 001e263ea8ffb..96ea621aefad1 100644 --- a/server/src/main/java/org/elasticsearch/index/store/Store.java +++ b/server/src/main/java/org/elasticsearch/index/store/Store.java @@ -360,18 +360,6 @@ public CheckIndex.Status checkIndex(PrintStream out) throws IOException { } } - /** - * Repairs the index using the previous returned status from {@link #checkIndex(PrintStream)}. - */ - public void exorciseIndex(CheckIndex.Status status) throws IOException { - metadataLock.writeLock().lock(); - try (CheckIndex checkIndex = new CheckIndex(directory)) { - checkIndex.exorciseIndex(status); - } finally { - metadataLock.writeLock().unlock(); - } - } - public StoreStats stats() throws IOException { ensureOpen(); return new StoreStats(directory.estimateSize()); diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/template/put/MetaDataIndexTemplateServiceTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/template/put/MetaDataIndexTemplateServiceTests.java index f0e9a57f7f3e6..39f04c6b7b092 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/template/put/MetaDataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/template/put/MetaDataIndexTemplateServiceTests.java @@ -69,7 +69,7 @@ public void testIndexTemplateInvalidNumberOfShards() { containsString("Failed to parse value [0] for setting [index.number_of_shards] must be >= 1")); assertThat(throwables.get(0).getMessage(), containsString("unknown value for [index.shard.check_on_startup] " + - "must be one of [true, false, fix, checksum] but was: blargh")); + "must be one of [true, false, checksum] but was: blargh")); } public void testIndexTemplateValidationAccumulatesValidationErrors() { diff --git a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java index 15e6151457fa2..1ac93babad193 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -22,11 +22,14 @@ import org.apache.lucene.index.CorruptIndexException; import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.IndexCommit; +import org.apache.lucene.index.IndexWriter; import org.apache.lucene.index.Term; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.TermQuery; import org.apache.lucene.search.TopDocs; import org.apache.lucene.store.AlreadyClosedException; +import org.apache.lucene.store.BaseDirectoryWrapper; +import org.apache.lucene.store.Directory; import org.apache.lucene.store.IOContext; import org.apache.lucene.util.Constants; import org.elasticsearch.Version; @@ -91,6 +94,7 @@ import org.elasticsearch.index.seqno.SeqNoStats; import org.elasticsearch.index.seqno.SequenceNumbers; import org.elasticsearch.index.snapshots.IndexShardSnapshotStatus; +import org.elasticsearch.index.store.DirectoryService; import org.elasticsearch.index.store.Store; import org.elasticsearch.index.store.StoreStats; import org.elasticsearch.index.translog.TestTranslog; @@ -108,6 +112,7 @@ import org.elasticsearch.snapshots.SnapshotId; import org.elasticsearch.snapshots.SnapshotInfo; import org.elasticsearch.snapshots.SnapshotShardFailure; +import org.elasticsearch.test.CorruptionUtils; import org.elasticsearch.test.DummyShardLock; import org.elasticsearch.test.FieldMaskingReader; import org.elasticsearch.test.VersionUtils; @@ -115,6 +120,7 @@ import java.io.IOException; import java.nio.charset.Charset; +import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; import java.util.Arrays; @@ -2480,13 +2486,66 @@ public void testReadSnapshotConcurrently() throws IOException, InterruptedExcept closeShards(newShard); } + public void testIndexCheckChecksum() throws Exception { + final boolean primary = true; + + IndexShard indexShard = newStartedShard(primary); + + final long numDocs = between(10, 100); + for (long i = 0; i < numDocs; i++) { + indexDoc(indexShard, "_doc", Long.toString(i), "{}"); + } + indexShard.flush(new FlushRequest()); + closeShards(indexShard); + + // corrupt files + final ShardPath shardPath = indexShard.shardPath(); + final Path dataPath = shardPath.getDataPath(); + final Path[] filesToCorrupt = + Files.walk(dataPath) + .filter(p -> Files.isRegularFile(p) && IndexWriter.WRITE_LOCK_NAME.equals(p.getFileName().toString()) == false) + .toArray(Path[]::new); + CorruptionUtils.corruptFile(random(), filesToCorrupt); + + final ShardRouting shardRouting = ShardRoutingHelper.initWithSameId(indexShard.routingEntry(), + primary ? RecoverySource.StoreRecoverySource.EXISTING_STORE_INSTANCE : RecoverySource.PeerRecoverySource.INSTANCE + ); + final IndexMetaData indexMetaData = IndexMetaData.builder(indexShard.indexSettings().getIndexMetaData()) + .settings(Settings.builder() + .put(indexShard.indexSettings.getSettings()) + .put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), "checksum")) + .build(); + + final IndexShard newShard = newShard(shardRouting, shardPath, indexMetaData, + null, indexShard.engineFactory, + indexSettings -> { + final ShardId shardId = shardPath.getShardId(); + final DirectoryService directoryService = new DirectoryService(shardId, indexSettings) { + @Override + public Directory newDirectory() throws IOException { + final BaseDirectoryWrapper baseDirectoryWrapper = newFSDirectory(shardPath.resolveIndex()); + baseDirectoryWrapper.setCheckIndexOnClose(false); + return baseDirectoryWrapper; + } + }; + return new Store(shardId, indexSettings, directoryService, new DummyShardLock(shardId)); + }, + indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); + + expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> newShard, primary)); + + closeShards(newShard); + } + /** * Simulates a scenario that happens when we are async fetching snapshot metadata from GatewayService * and checking index concurrently. This should always be possible without any exception. */ public void testReadSnapshotAndCheckIndexConcurrently() throws Exception { final boolean isPrimary = randomBoolean(); + IndexShard indexShard = newStartedShard(isPrimary); + final long numDocs = between(10, 100); for (long i = 0; i < numDocs; i++) { indexDoc(indexShard, "_doc", Long.toString(i), "{}"); @@ -2503,7 +2562,7 @@ public void testReadSnapshotAndCheckIndexConcurrently() throws Exception { final IndexMetaData indexMetaData = IndexMetaData.builder(indexShard.indexSettings().getIndexMetaData()) .settings(Settings.builder() .put(indexShard.indexSettings.getSettings()) - .put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), randomFrom("false", "true", "checksum", "fix"))) + .put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), randomFrom("false", "true", "checksum"))) .build(); final IndexShard newShard = newShard(shardRouting, indexShard.shardPath(), indexMetaData, null, indexShard.engineFactory, indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); diff --git a/test/framework/src/main/java/org/elasticsearch/index/shard/IndexShardTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/shard/IndexShardTestCase.java index 0cbc6e44502fe..ae13e3a8f5abc 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/shard/IndexShardTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/shard/IndexShardTestCase.java @@ -37,6 +37,7 @@ import org.elasticsearch.cluster.routing.ShardRoutingHelper; import org.elasticsearch.cluster.routing.ShardRoutingState; import org.elasticsearch.cluster.routing.TestShardRouting; +import org.elasticsearch.common.CheckedFunction; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.lucene.uid.Versions; @@ -179,12 +180,23 @@ public Directory newDirectory() throws IOException { * * @param primary indicates whether to a primary shard (ready to recover from an empty store) or a replica * (ready to recover from another shard) + * @param indexSettings the {@link Settings} to use for this index */ - protected IndexShard newShard(boolean primary) throws IOException { + protected IndexShard newShard(boolean primary, Settings indexSettings) throws IOException { ShardRouting shardRouting = TestShardRouting.newShardRouting(new ShardId("index", "_na_", 0), randomAlphaOfLength(10), primary, ShardRoutingState.INITIALIZING, primary ? RecoverySource.StoreRecoverySource.EMPTY_STORE_INSTANCE : RecoverySource.PeerRecoverySource.INSTANCE); - return newShard(shardRouting); + return newShard(shardRouting, indexSettings); + } + + /** + * creates a new initializing shard. The shard will have its own unique data path. + * + * @param primary indicates whether to a primary shard (ready to recover from an empty store) or a replica + * (ready to recover from another shard) + */ + protected IndexShard newShard(boolean primary) throws IOException { + return newShard(primary, Settings.EMPTY); } /** @@ -193,13 +205,29 @@ protected IndexShard newShard(boolean primary) throws IOException { * @param shardRouting the {@link ShardRouting} to use for this shard * @param listeners an optional set of listeners to add to the shard */ + protected IndexShard newShard( + final ShardRouting shardRouting, + final IndexingOperationListener... listeners) throws IOException { + return newShard(shardRouting, Settings.EMPTY, listeners); + } + + /** + * creates a new initializing shard. The shard will have its own unique data path. + * + * @param shardRouting the {@link ShardRouting} to use for this shard + * @param indexSettings the {@link Settings} to use for this index + * @param listeners an optional set of listeners to add to the shard + */ protected IndexShard newShard( final ShardRouting shardRouting, + final Settings indexSettings, final IndexingOperationListener... listeners) throws IOException { assert shardRouting.initializing() : shardRouting; - Settings settings = Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT) + Settings settings = Settings.builder() + .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT) .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) + .put(indexSettings) .build(); IndexMetaData.Builder metaData = IndexMetaData.builder(shardRouting.getIndexName()) .settings(settings) @@ -303,10 +331,32 @@ protected IndexShard newShard(ShardRouting routing, ShardPath shardPath, IndexMe @Nullable EngineFactory engineFactory, Runnable globalCheckpointSyncer, IndexEventListener indexEventListener, IndexingOperationListener... listeners) throws IOException { + return newShard(routing, shardPath, indexMetaData, indexSearcherWrapper, engineFactory, + indexSettings -> createStore(indexSettings, shardPath), + globalCheckpointSyncer, + indexEventListener, listeners); + } + + /** + * creates a new initializing shard. + * @param routing shard routing to use + * @param shardPath path to use for shard data + * @param indexMetaData indexMetaData for the shard, including any mapping + * @param indexSearcherWrapper an optional wrapper to be used during searchers + * @param globalCheckpointSyncer callback for syncing global checkpoints + * @param indexEventListener index even listener + * @param listeners an optional set of listeners to add to the shard + */ + protected IndexShard newShard(ShardRouting routing, ShardPath shardPath, IndexMetaData indexMetaData, + @Nullable IndexSearcherWrapper indexSearcherWrapper, + @Nullable EngineFactory engineFactory, + CheckedFunction storeProvider, + Runnable globalCheckpointSyncer, + IndexEventListener indexEventListener, IndexingOperationListener... listeners) throws IOException { final Settings nodeSettings = Settings.builder().put("node.name", routing.currentNodeId()).build(); final IndexSettings indexSettings = new IndexSettings(indexMetaData, nodeSettings); final IndexShard indexShard; - final Store store = createStore(indexSettings, shardPath); + final Store store = storeProvider.apply(indexSettings); boolean success = false; try { IndexCache indexCache = new IndexCache(indexSettings, new DisabledQueryCache(indexSettings), null); @@ -375,7 +425,18 @@ protected IndexShard newStartedShard() throws IOException { * @param primary controls whether the shard will be a primary or a replica. */ protected IndexShard newStartedShard(boolean primary) throws IOException { - IndexShard shard = newShard(primary); + return newStartedShard(p -> newShard(p), primary); + } + + /** + * creates a new empty shard and starts it. + * + * @param shardFunction shard factory function + * @param primary controls whether the shard will be a primary or a replica. + */ + protected IndexShard newStartedShard(CheckedFunction shardFunction, + boolean primary) throws IOException { + IndexShard shard = shardFunction.apply(primary); if (primary) { recoverShardFromStore(shard); } else {