From 843f97795a4ff5fd475b131d567d6603b1ba9f34 Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Mon, 23 Jul 2018 13:08:03 +0200 Subject: [PATCH 01/12] drop `index.shard.check_on_startup: fix` Relates #31389 --- docs/reference/index-modules.asciidoc | 6 - .../elasticsearch/index/IndexSettings.java | 3 +- .../elasticsearch/index/shard/IndexShard.java | 18 +-- .../org/elasticsearch/index/store/Store.java | 15 +-- .../MetaDataIndexTemplateServiceTests.java | 2 +- .../index/shard/IndexShardTests.java | 111 +++++++++++++++++- .../index/shard/IndexShardTestCase.java | 59 ++++++++-- 7 files changed, 167 insertions(+), 47 deletions(-) diff --git a/docs/reference/index-modules.asciidoc b/docs/reference/index-modules.asciidoc index 54c0c1c1b157c..53de67e55fdf9 100644 --- a/docs/reference/index-modules.asciidoc +++ b/docs/reference/index-modules.asciidoc @@ -63,12 +63,6 @@ corruption is detected, it will prevent the shard from being opened. Accepts: Check for both physical and logical corruption. This is much more expensive in terms of CPU and memory usage. -`fix`:: - - Check for both physical and logical corruption. Segments that were reported - as corrupted will be automatically removed. This option *may result in data loss*. - Use with extreme caution! - WARNING: Expert only. Checking shards may take a lot of time on large indices. -- diff --git a/server/src/main/java/org/elasticsearch/index/IndexSettings.java b/server/src/main/java/org/elasticsearch/index/IndexSettings.java index 486515e675562..e521814764087 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 d4a1d0502d0aa..f8ce7d2ae99a7 100644 --- a/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java +++ b/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java @@ -1298,7 +1298,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) { @@ -1890,6 +1890,9 @@ void checkIndex() throws IOException { if (store.tryIncRef()) { try { doCheckIndex(); + } catch (IOException e) { + store.markStoreCorrupted(e); + throw e; } finally { store.decRef(); } @@ -1933,18 +1936,7 @@ 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"); - } + throw new IllegalStateException("index check failure"); } } 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..aab177d2f913a 100644 --- a/server/src/main/java/org/elasticsearch/index/store/Store.java +++ b/server/src/main/java/org/elasticsearch/index/store/Store.java @@ -134,7 +134,8 @@ public class Store extends AbstractIndexShardComponent implements Closeable, Ref static final int VERSION_STACK_TRACE = 1; // we write the stack trace too since 1.4.0 static final int VERSION_START = 0; static final int VERSION = VERSION_WRITE_THROWABLE; - static final String CORRUPTED = "corrupted_"; + // public is for test purposes + public static final String CORRUPTED = "corrupted_"; public static final Setting INDEX_STORE_STATS_REFRESH_INTERVAL_SETTING = Setting.timeSetting("index.store.stats_refresh_interval", TimeValue.timeValueSeconds(10), Property.IndexScope); @@ -360,18 +361,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 1880b6b0954dc..241727ee2f1d4 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,13 @@ 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.FilterDirectory; import org.apache.lucene.store.IOContext; @@ -53,6 +55,7 @@ import org.elasticsearch.cluster.routing.ShardRoutingState; import org.elasticsearch.cluster.routing.TestShardRouting; import org.elasticsearch.cluster.routing.UnassignedInfo; +import org.elasticsearch.common.CheckedFunction; import org.elasticsearch.common.Strings; import org.elasticsearch.common.UUIDs; import org.elasticsearch.common.breaker.CircuitBreaker; @@ -93,6 +96,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; @@ -110,6 +114,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; @@ -118,7 +123,11 @@ import java.io.IOException; import java.nio.charset.Charset; +import java.nio.file.FileVisitResult; +import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.attribute.BasicFileAttributes; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -1216,7 +1225,7 @@ public String[] listAll() throws IOException { }; try (Store store = createStore(shardId, new IndexSettings(metaData, Settings.EMPTY), directory)) { - IndexShard shard = newShard(shardRouting, shardPath, metaData, store, + IndexShard shard = newShard(shardRouting, shardPath, metaData, i -> store, null, new InternalEngineFactory(), () -> { }, EMPTY_EVENT_LISTENER); AtomicBoolean failureCallbackTriggered = new AtomicBoolean(false); @@ -2561,6 +2570,104 @@ 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); + + // start shard with checksum - it has to pass successfully + + 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 ShardPath shardPath = indexShard.shardPath(); + + final IndexShard newShard = newShard(shardRouting, shardPath, indexMetaData, + null, null, indexShard.engineFactory, + indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); + + closeShards(newStartedShard(p -> newShard, primary)); + + // corrupt files + final Path indexPath = shardPath.getDataPath().resolve("index"); + final Path[] filesToCorrupt = + Files.walk(indexPath) + .filter(p -> Files.isRegularFile(p) && IndexWriter.WRITE_LOCK_NAME.equals(p.getFileName().toString()) == false) + .toArray(Path[]::new); + CorruptionUtils.corruptFile(random(), filesToCorrupt); + + // check that corrupt marker is *NOT* there + final AtomicInteger corruptedMarkerCount = new AtomicInteger(); + final SimpleFileVisitor corruptedVisitor = new SimpleFileVisitor() { + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + if (Files.isRegularFile(file) && file.getFileName().toString().startsWith(Store.CORRUPTED)) { + corruptedMarkerCount.incrementAndGet(); + } + return FileVisitResult.CONTINUE; + } + }; + Files.walkFileTree(indexPath, corruptedVisitor); + + assertThat("store is clean", + corruptedMarkerCount.get(), equalTo(0)); + + // storeProvider that does not perform check index on close - it is corrupted + final CheckedFunction storeProvider = 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()); + // index is corrupted - don't even try to check index on close - it fails + baseDirectoryWrapper.setCheckIndexOnClose(false); + return baseDirectoryWrapper; + } + }; + return new Store(shardId, indexSettings, directoryService, new DummyShardLock(shardId)); + }; + + // try to start shard on corrupted files + final IndexShard corruptedShard = newShard(shardRouting, shardPath, indexMetaData, + storeProvider, null, indexShard.engineFactory, + indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); + + expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard, primary)); + closeShards(corruptedShard); + + // check that corrupt marker is there + Files.walkFileTree(indexPath, corruptedVisitor); + assertThat("store has to be marked as corrupted", + corruptedMarkerCount.get(), equalTo(1)); + + // try to start another time shard on corrupted files + final IndexShard corruptedShard2 = newShard(shardRouting, shardPath, indexMetaData, + storeProvider, null, indexShard.engineFactory, + indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); + + expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard2, primary)); + closeShards(corruptedShard2); + + // check that corrupt marker is there + corruptedMarkerCount.set(0); + Files.walkFileTree(indexPath, corruptedVisitor); + assertThat("store still has a single corrupt marker", + corruptedMarkerCount.get(), equalTo(1)); + } + /** * 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. @@ -2584,7 +2691,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, 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 f9289f658614c..d596b3ca4ba6e 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; @@ -163,7 +164,6 @@ public Settings threadPoolSettings() { return Settings.EMPTY; } - protected Store createStore(IndexSettings indexSettings, ShardPath shardPath) throws IOException { return createStore(shardPath.getShardId(), indexSettings, newFSDirectory(shardPath.resolveIndex())); } @@ -176,7 +176,6 @@ public Directory newDirectory() throws IOException { } }; return new Store(shardId, indexSettings, directoryService, new DummyShardLock(shardId)); - } /** @@ -184,12 +183,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); } /** @@ -198,13 +208,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) @@ -298,23 +324,25 @@ protected IndexShard newShard(ShardRouting routing, IndexMetaData indexMetaData, * @param routing shard routing to use * @param shardPath path to use for shard data * @param indexMetaData indexMetaData for the shard, including any mapping - * @param store an optional custom store to use. If null a default file based store will be created + * @param storeProvider an optional custom store provider to use. If null a default file based store will be created * @param indexSearcherWrapper an optional wrapper to be used during searchers * @param globalCheckpointSyncer callback for syncing global checkpoints * @param indexEventListener index event listener * @param listeners an optional set of listeners to add to the shard */ protected IndexShard newShard(ShardRouting routing, ShardPath shardPath, IndexMetaData indexMetaData, - @Nullable Store store, @Nullable IndexSearcherWrapper indexSearcherWrapper, + @Nullable CheckedFunction storeProvider, + @Nullable IndexSearcherWrapper indexSearcherWrapper, @Nullable EngineFactory engineFactory, 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; - if (store == null) { - store = createStore(indexSettings, shardPath); + if (storeProvider == null) { + storeProvider = is -> createStore(is, shardPath); } + final Store store = storeProvider.apply(indexSettings); boolean success = false; try { IndexCache indexCache = new IndexCache(indexSettings, new DisabledQueryCache(indexSettings), null); @@ -384,7 +412,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 { From 153e4f25c55981d6cdb4d576f3d1fd45d0552315 Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Tue, 21 Aug 2018 11:40:34 +0200 Subject: [PATCH 02/12] create corrupted marker on `check_on_startup: true`; split testIndexCheckChecksum into two tests: testIndexCheckOnStartup, testShardDoesNotStartIfCorruptedMarkerIsPresent; clean up --- .../elasticsearch/index/shard/IndexShard.java | 2 +- .../index/shard/IndexShardTests.java | 127 ++++++++++++------ 2 files changed, 85 insertions(+), 44 deletions(-) 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 f8ce7d2ae99a7..de3a47a398fee 100644 --- a/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java +++ b/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java @@ -1936,7 +1936,7 @@ private void doCheckIndex() throws IOException { return; } logger.warn("check index [failure]\n{}", os.bytes().utf8ToString()); - throw new IllegalStateException("index check failure"); + throw new IOException("index check failure"); } } 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 a36210e63f834..00a5ff1acfde4 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -28,7 +28,6 @@ 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.FilterDirectory; import org.apache.lucene.store.IOContext; @@ -55,7 +54,6 @@ import org.elasticsearch.cluster.routing.ShardRoutingState; import org.elasticsearch.cluster.routing.TestShardRouting; import org.elasticsearch.cluster.routing.UnassignedInfo; -import org.elasticsearch.common.CheckedFunction; import org.elasticsearch.common.Strings; import org.elasticsearch.common.UUIDs; import org.elasticsearch.common.breaker.CircuitBreaker; @@ -96,7 +94,6 @@ 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; @@ -2567,10 +2564,8 @@ public void testReadSnapshotConcurrently() throws IOException, InterruptedExcept closeShards(newShard); } - public void testIndexCheckChecksum() throws Exception { - final boolean primary = true; - - IndexShard indexShard = newStartedShard(primary); + public void testIndexCheckOnStartup() throws Exception { + final IndexShard indexShard = newStartedShard(true); final long numDocs = between(10, 100); for (long i = 0; i < numDocs; i++) { @@ -2580,31 +2575,18 @@ public void testIndexCheckChecksum() throws Exception { closeShards(indexShard); // start shard with checksum - it has to pass successfully - final ShardRouting shardRouting = ShardRoutingHelper.initWithSameId(indexShard.routingEntry(), - primary ? RecoverySource.StoreRecoverySource.EXISTING_STORE_INSTANCE : RecoverySource.PeerRecoverySource.INSTANCE + RecoverySource.StoreRecoverySource.EXISTING_STORE_INSTANCE ); final IndexMetaData indexMetaData = IndexMetaData.builder(indexShard.indexSettings().getIndexMetaData()) .settings(Settings.builder() .put(indexShard.indexSettings.getSettings()) - .put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), "checksum")) + .put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), randomFrom("true", "checksum"))) .build(); final ShardPath shardPath = indexShard.shardPath(); - final IndexShard newShard = newShard(shardRouting, shardPath, indexMetaData, - null, null, indexShard.engineFactory, - indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); - - closeShards(newStartedShard(p -> newShard, primary)); - - // corrupt files - final Path indexPath = shardPath.getDataPath().resolve("index"); - final Path[] filesToCorrupt = - Files.walk(indexPath) - .filter(p -> Files.isRegularFile(p) && IndexWriter.WRITE_LOCK_NAME.equals(p.getFileName().toString()) == false) - .toArray(Path[]::new); - CorruptionUtils.corruptFile(random(), filesToCorrupt); + final Path indexPath = corruptIndexFile(shardPath); // check that corrupt marker is *NOT* there final AtomicInteger corruptedMarkerCount = new AtomicInteger(); @@ -2622,41 +2604,85 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO assertThat("store is clean", corruptedMarkerCount.get(), equalTo(0)); - // storeProvider that does not perform check index on close - it is corrupted - final CheckedFunction storeProvider = 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()); - // index is corrupted - don't even try to check index on close - it fails - baseDirectoryWrapper.setCheckIndexOnClose(false); - return baseDirectoryWrapper; - } - }; - return new Store(shardId, indexSettings, directoryService, new DummyShardLock(shardId)); - }; + IndexShard corruptedShard = newShard(shardRouting, shardPath, indexMetaData, + null, null, indexShard.engineFactory, + indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); + + expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard, true)); + + // check that corrupt marker is there + Files.walkFileTree(indexPath, corruptedVisitor); + assertThat("store has to be marked as corrupted", + corruptedMarkerCount.get(), equalTo(1)); + + try { + closeShards(corruptedShard); + } catch (RuntimeException e) { + assertThat(e.getMessage(), equalTo("CheckIndex failed")); + } + } + + public void testShardDoesNotStartIfCorruptedMarkerIsPresent() throws Exception { + final IndexShard indexShard = newStartedShard(true); + + 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); + + final ShardPath shardPath = indexShard.shardPath(); + + final ShardRouting shardRouting = ShardRoutingHelper.initWithSameId(indexShard.routingEntry(), + RecoverySource.StoreRecoverySource.EXISTING_STORE_INSTANCE + ); + final IndexMetaData indexMetaData = IndexMetaData.builder(indexShard.indexSettings().getIndexMetaData()) + .settings(Settings.builder() + .put(indexShard.indexSettings.getSettings()) + .put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), randomFrom("true", "checksum"))) + .build(); + + final Path indexPath = corruptIndexFile(shardPath); // try to start shard on corrupted files final IndexShard corruptedShard = newShard(shardRouting, shardPath, indexMetaData, - storeProvider, null, indexShard.engineFactory, + null, null, indexShard.engineFactory, indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); - expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard, primary)); - closeShards(corruptedShard); + expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard, true)); + try { + closeShards(corruptedShard); + } catch (RuntimeException e) { + assertThat(e.getMessage(), equalTo("CheckIndex failed")); + } // check that corrupt marker is there + final AtomicInteger corruptedMarkerCount = new AtomicInteger(); + final SimpleFileVisitor corruptedVisitor = new SimpleFileVisitor() { + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + if (Files.isRegularFile(file) && file.getFileName().toString().startsWith(Store.CORRUPTED)) { + corruptedMarkerCount.incrementAndGet(); + } + return FileVisitResult.CONTINUE; + } + }; Files.walkFileTree(indexPath, corruptedVisitor); assertThat("store has to be marked as corrupted", corruptedMarkerCount.get(), equalTo(1)); // try to start another time shard on corrupted files final IndexShard corruptedShard2 = newShard(shardRouting, shardPath, indexMetaData, - storeProvider, null, indexShard.engineFactory, + null, null, indexShard.engineFactory, indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); - expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard2, primary)); - closeShards(corruptedShard2); + expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard2, true)); + try { + closeShards(corruptedShard2); + } catch (RuntimeException e) { + assertThat(e.getMessage(), equalTo("CheckIndex failed")); + } // check that corrupt marker is there corruptedMarkerCount.set(0); @@ -2665,6 +2691,21 @@ public Directory newDirectory() throws IOException { corruptedMarkerCount.get(), equalTo(1)); } + private Path corruptIndexFile(ShardPath shardPath) throws IOException { + final Path indexPath = shardPath.getDataPath().resolve(ShardPath.INDEX_FOLDER_NAME); + final Path[] filesToCorrupt = + Files.walk(indexPath) + .filter(p -> { + final String name = p.getFileName().toString(); + return Files.isRegularFile(p) + && IndexWriter.WRITE_LOCK_NAME.equals(name) == false + && name.startsWith("segments_") == false && name.endsWith(".si") == false; + }) + .toArray(Path[]::new); + CorruptionUtils.corruptFile(random(), filesToCorrupt); + return indexPath; + } + /** * 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. From c71e30672a0e7709af3a9dfe11fe04ed2f714b78 Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Tue, 21 Aug 2018 16:35:34 +0200 Subject: [PATCH 03/12] create manually corruption marker (but don't corrupt index files) to check shard does not start if corruption marker is there; drop unnecessary new lines --- .../index/shard/IndexShardTests.java | 46 ++++++++----------- 1 file changed, 20 insertions(+), 26 deletions(-) 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 f44596cf54018..a7c51eb8f45b9 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -2614,8 +2614,7 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO }; Files.walkFileTree(indexPath, corruptedVisitor); - assertThat("store is clean", - corruptedMarkerCount.get(), equalTo(0)); + assertThat("store is clean", corruptedMarkerCount.get(), equalTo(0)); IndexShard corruptedShard = newShard(shardRouting, shardPath, indexMetaData, null, null, indexShard.engineFactory, @@ -2625,8 +2624,7 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO // check that corrupt marker is there Files.walkFileTree(indexPath, corruptedVisitor); - assertThat("store has to be marked as corrupted", - corruptedMarkerCount.get(), equalTo(1)); + assertThat("store has to be marked as corrupted", corruptedMarkerCount.get(), equalTo(1)); try { closeShards(corruptedShard); @@ -2650,25 +2648,25 @@ public void testShardDoesNotStartIfCorruptedMarkerIsPresent() throws Exception { final ShardRouting shardRouting = ShardRoutingHelper.initWithSameId(indexShard.routingEntry(), RecoverySource.StoreRecoverySource.EXISTING_STORE_INSTANCE ); - final IndexMetaData indexMetaData = IndexMetaData.builder(indexShard.indexSettings().getIndexMetaData()) - .settings(Settings.builder() - .put(indexShard.indexSettings.getSettings()) - .put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), randomFrom("true", "checksum"))) - .build(); + final IndexMetaData indexMetaData = indexShard.indexSettings().getIndexMetaData(); - final Path indexPath = corruptIndexFile(shardPath); + final Path indexPath = shardPath.getDataPath().resolve(ShardPath.INDEX_FOLDER_NAME); + + // create corrupted marker + final String corruptionMessage = "fake ioexception"; + try(final Store store = createStore(indexShard.indexSettings(), shardPath)){ + store.markStoreCorrupted(new IOException(corruptionMessage)); + } // try to start shard on corrupted files final IndexShard corruptedShard = newShard(shardRouting, shardPath, indexMetaData, null, null, indexShard.engineFactory, indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); - expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard, true)); - try { - closeShards(corruptedShard); - } catch (RuntimeException e) { - assertThat(e.getMessage(), equalTo("CheckIndex failed")); - } + final IndexShardRecoveryException exception1 = expectThrows(IndexShardRecoveryException.class, + () -> newStartedShard(p -> corruptedShard, true)); + assertThat(exception1.getCause().getMessage(), equalTo(corruptionMessage + " (resource=preexisting_corruption)")); + closeShards(corruptedShard); // check that corrupt marker is there final AtomicInteger corruptedMarkerCount = new AtomicInteger(); @@ -2682,26 +2680,22 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO } }; Files.walkFileTree(indexPath, corruptedVisitor); - assertThat("store has to be marked as corrupted", - corruptedMarkerCount.get(), equalTo(1)); + assertThat("store has to be marked as corrupted", corruptedMarkerCount.get(), equalTo(1)); // try to start another time shard on corrupted files final IndexShard corruptedShard2 = newShard(shardRouting, shardPath, indexMetaData, null, null, indexShard.engineFactory, indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); - expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard2, true)); - try { - closeShards(corruptedShard2); - } catch (RuntimeException e) { - assertThat(e.getMessage(), equalTo("CheckIndex failed")); - } + final IndexShardRecoveryException exception2 = expectThrows(IndexShardRecoveryException.class, + () -> newStartedShard(p -> corruptedShard2, true)); + assertThat(exception2.getCause().getMessage(), equalTo(corruptionMessage + " (resource=preexisting_corruption)")); + closeShards(corruptedShard2); // check that corrupt marker is there corruptedMarkerCount.set(0); Files.walkFileTree(indexPath, corruptedVisitor); - assertThat("store still has a single corrupt marker", - corruptedMarkerCount.get(), equalTo(1)); + assertThat("store still has a single corrupt marker", corruptedMarkerCount.get(), equalTo(1)); } private Path corruptIndexFile(ShardPath shardPath) throws IOException { From a7668d6d02c7f3baaa11f7a46ff1fc18e4b0a90d Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Tue, 21 Aug 2018 17:02:26 +0200 Subject: [PATCH 04/12] checkstyle fix --- .../java/org/elasticsearch/index/shard/IndexShardTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 a7c51eb8f45b9..100851f8f847f 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -2654,7 +2654,7 @@ public void testShardDoesNotStartIfCorruptedMarkerIsPresent() throws Exception { // create corrupted marker final String corruptionMessage = "fake ioexception"; - try(final Store store = createStore(indexShard.indexSettings(), shardPath)){ + try(Store store = createStore(indexShard.indexSettings(), shardPath)){ store.markStoreCorrupted(new IOException(corruptionMessage)); } From c155b360334ff14958f2baa5fbeca78c17e2ad29 Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Mon, 27 Aug 2018 12:26:23 +0200 Subject: [PATCH 05/12] addressed unit test comments --- .../index/shard/IndexShardTests.java | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) 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 100851f8f847f..62d27816605cb 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -2587,21 +2587,10 @@ public void testIndexCheckOnStartup() throws Exception { indexShard.flush(new FlushRequest()); closeShards(indexShard); - // start shard with checksum - it has to pass successfully - final ShardRouting shardRouting = ShardRoutingHelper.initWithSameId(indexShard.routingEntry(), - RecoverySource.StoreRecoverySource.EXISTING_STORE_INSTANCE - ); - final IndexMetaData indexMetaData = IndexMetaData.builder(indexShard.indexSettings().getIndexMetaData()) - .settings(Settings.builder() - .put(indexShard.indexSettings.getSettings()) - .put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), randomFrom("true", "checksum"))) - .build(); - final ShardPath shardPath = indexShard.shardPath(); final Path indexPath = corruptIndexFile(shardPath); - // check that corrupt marker is *NOT* there final AtomicInteger corruptedMarkerCount = new AtomicInteger(); final SimpleFileVisitor corruptedVisitor = new SimpleFileVisitor() { @Override @@ -2614,13 +2603,25 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO }; Files.walkFileTree(indexPath, corruptedVisitor); - assertThat("store is clean", corruptedMarkerCount.get(), equalTo(0)); + assertThat("corruption marker should not be there", corruptedMarkerCount.get(), equalTo(0)); + + final ShardRouting shardRouting = ShardRoutingHelper.initWithSameId(indexShard.routingEntry(), + RecoverySource.StoreRecoverySource.EXISTING_STORE_INSTANCE + ); + // start shard and perform index check on startup. It enforce shard to fail due to corrupted index files + final IndexMetaData indexMetaData = IndexMetaData.builder(indexShard.indexSettings().getIndexMetaData()) + .settings(Settings.builder() + .put(indexShard.indexSettings.getSettings()) + .put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), randomFrom("true", "checksum"))) + .build(); IndexShard corruptedShard = newShard(shardRouting, shardPath, indexMetaData, null, null, indexShard.engineFactory, indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); - expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard, true)); + final IndexShardRecoveryException indexShardRecoveryException = + expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard, true)); + assertThat(indexShardRecoveryException.getMessage(), equalTo("failed recovery")); // check that corrupt marker is there Files.walkFileTree(indexPath, corruptedVisitor); @@ -2654,7 +2655,7 @@ public void testShardDoesNotStartIfCorruptedMarkerIsPresent() throws Exception { // create corrupted marker final String corruptionMessage = "fake ioexception"; - try(Store store = createStore(indexShard.indexSettings(), shardPath)){ + try(Store store = createStore(indexShard.indexSettings(), shardPath)) { store.markStoreCorrupted(new IOException(corruptionMessage)); } @@ -2668,7 +2669,6 @@ public void testShardDoesNotStartIfCorruptedMarkerIsPresent() throws Exception { assertThat(exception1.getCause().getMessage(), equalTo(corruptionMessage + " (resource=preexisting_corruption)")); closeShards(corruptedShard); - // check that corrupt marker is there final AtomicInteger corruptedMarkerCount = new AtomicInteger(); final SimpleFileVisitor corruptedVisitor = new SimpleFileVisitor() { @Override From 85b7eefab7db3acec571bf63c6262cc33bb77f9e Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Mon, 27 Aug 2018 14:02:32 +0200 Subject: [PATCH 06/12] keep `fix` for 6.x branch --- docs/reference/index-modules.asciidoc | 4 ++++ .../src/main/java/org/elasticsearch/index/IndexSettings.java | 3 ++- .../template/put/MetaDataIndexTemplateServiceTests.java | 2 +- .../java/org/elasticsearch/index/shard/IndexShardTests.java | 2 +- 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/docs/reference/index-modules.asciidoc b/docs/reference/index-modules.asciidoc index 53de67e55fdf9..214d77541779e 100644 --- a/docs/reference/index-modules.asciidoc +++ b/docs/reference/index-modules.asciidoc @@ -63,6 +63,10 @@ corruption is detected, it will prevent the shard from being opened. Accepts: Check for both physical and logical corruption. This is much more expensive in terms of CPU and memory usage. +`fix`:: + + The same as `false`. This option is deprecated and will be completely removed in 7.0. + WARNING: Expert only. Checking shards may take a lot of time on large indices. -- diff --git a/server/src/main/java/org/elasticsearch/index/IndexSettings.java b/server/src/main/java/org/elasticsearch/index/IndexSettings.java index bcdf48d4b7649..44cd743bbd428 100644 --- a/server/src/main/java/org/elasticsearch/index/IndexSettings.java +++ b/server/src/main/java/org/elasticsearch/index/IndexSettings.java @@ -75,10 +75,11 @@ 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, checksum] but was: " + s); + throw new IllegalArgumentException("unknown value for [index.shard.check_on_startup] must be one of [true, false, fix, checksum] but was: " + s); } }, Property.IndexScope); 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 39f04c6b7b092..f0e9a57f7f3e6 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, checksum] but was: blargh")); + "must be one of [true, false, fix, 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 62d27816605cb..2f89388b0a64e 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -2736,7 +2736,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"))) + .put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), randomFrom("false", "true", "checksum", "fix"))) .build(); final IndexShard newShard = newShard(shardRouting, indexShard.shardPath(), indexMetaData, null, null, indexShard.engineFactory, indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); From 323180391c4011b48e00888ad1762beeec451bbf Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Tue, 28 Aug 2018 13:46:40 +0200 Subject: [PATCH 07/12] added `fix` deprecation log message + test --- .../elasticsearch/index/shard/IndexShard.java | 4 ++ .../IndexShardDeprecatedSettingTests.java | 54 +++++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 server/src/test/java/org/elasticsearch/index/shard/IndexShardDeprecatedSettingTests.java 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 da17af45db576..e0c821b8350c8 100644 --- a/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java +++ b/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java @@ -299,6 +299,10 @@ public IndexShard( logger.debug("state: [CREATED]"); this.checkIndexOnStartup = indexSettings.getValue(IndexSettings.INDEX_CHECK_ON_STARTUP); + if ("fix".equals(checkIndexOnStartup)) { + deprecationLogger.deprecated("Setting [index.shard.check_on_startup] is set to deprecated value [fix], " + + "which will be unsupported in future"); + } this.translogConfig = new TranslogConfig(shardId, shardPath().resolveTranslog(), indexSettings, bigArrays); final String aId = shardRouting.allocationId().getId(); this.globalCheckpointListeners = new GlobalCheckpointListeners(shardId, threadPool.executor(ThreadPool.Names.LISTENER), logger); diff --git a/server/src/test/java/org/elasticsearch/index/shard/IndexShardDeprecatedSettingTests.java b/server/src/test/java/org/elasticsearch/index/shard/IndexShardDeprecatedSettingTests.java new file mode 100644 index 0000000000000..260a507b80783 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardDeprecatedSettingTests.java @@ -0,0 +1,54 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.index.shard; + +import org.elasticsearch.common.logging.DeprecationLogger; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.index.IndexSettings; + +import java.util.List; +import java.util.Map; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.hasSize; + +public class IndexShardDeprecatedSettingTests extends IndexShardTestCase { + @Override + protected boolean enableWarningsCheck() { + return false; + } + + public void testCheckOnStartupDeprecatedValue() throws Exception { + final Settings settings = Settings.builder().put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), "fix").build(); + + try(ThreadContext threadContext = new ThreadContext(Settings.EMPTY)) { + DeprecationLogger.setThreadContext(threadContext); + final IndexShard newShard = newShard(true, settings); + + final Map> responseHeaders = threadContext.getResponseHeaders(); + final List warnings = responseHeaders.get("Warning"); + assertThat(warnings.toString(), warnings, hasSize(1)); + assertThat(warnings.get(0), containsString("Setting [index.shard.check_on_startup] is set to deprecated value [fix], " + + "which will be unsupported in future")); + + closeShards(newShard); + } + } +} From c2b5b8a2f344bac4bb569f16f41a7333a841b13d Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Tue, 28 Aug 2018 14:31:57 +0200 Subject: [PATCH 08/12] added `fix` deprecation log message + test --- .../IndexShardDeprecatedSettingTests.java | 54 ------------------- .../index/shard/IndexShardTests.java | 10 ++++ 2 files changed, 10 insertions(+), 54 deletions(-) delete mode 100644 server/src/test/java/org/elasticsearch/index/shard/IndexShardDeprecatedSettingTests.java diff --git a/server/src/test/java/org/elasticsearch/index/shard/IndexShardDeprecatedSettingTests.java b/server/src/test/java/org/elasticsearch/index/shard/IndexShardDeprecatedSettingTests.java deleted file mode 100644 index 260a507b80783..0000000000000 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardDeprecatedSettingTests.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.elasticsearch.index.shard; - -import org.elasticsearch.common.logging.DeprecationLogger; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.util.concurrent.ThreadContext; -import org.elasticsearch.index.IndexSettings; - -import java.util.List; -import java.util.Map; - -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.hasSize; - -public class IndexShardDeprecatedSettingTests extends IndexShardTestCase { - @Override - protected boolean enableWarningsCheck() { - return false; - } - - public void testCheckOnStartupDeprecatedValue() throws Exception { - final Settings settings = Settings.builder().put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), "fix").build(); - - try(ThreadContext threadContext = new ThreadContext(Settings.EMPTY)) { - DeprecationLogger.setThreadContext(threadContext); - final IndexShard newShard = newShard(true, settings); - - final Map> responseHeaders = threadContext.getResponseHeaders(); - final List warnings = responseHeaders.get("Warning"); - assertThat(warnings.toString(), warnings, hasSize(1)); - assertThat(warnings.get(0), containsString("Setting [index.shard.check_on_startup] is set to deprecated value [fix], " - + "which will be unsupported in future")); - - closeShards(newShard); - } - } -} 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 2f89388b0a64e..12f7b85bd185e 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -2778,6 +2778,16 @@ public void testReadSnapshotAndCheckIndexConcurrently() throws Exception { closeShards(newShard); } + public void testCheckOnStartupDeprecatedValue() throws Exception { + final Settings settings = Settings.builder().put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), "fix").build(); + + final IndexShard newShard = newShard(true, settings); + closeShards(newShard); + + assertWarnings("Setting [index.shard.check_on_startup] is set to deprecated value [fix], " + + "which will be unsupported in future"); + } + class Result { private final int localCheckpoint; private final int maxSeqNo; From 14e6175923ca6bc30f3f98aa91753a3a8a210250 Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Tue, 28 Aug 2018 14:34:53 +0200 Subject: [PATCH 09/12] adjusted `fix` deprecation log message --- .../src/main/java/org/elasticsearch/index/shard/IndexShard.java | 2 +- .../java/org/elasticsearch/index/shard/IndexShardTests.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 e0c821b8350c8..f663bb6ffc43c 100644 --- a/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java +++ b/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java @@ -301,7 +301,7 @@ public IndexShard( this.checkIndexOnStartup = indexSettings.getValue(IndexSettings.INDEX_CHECK_ON_STARTUP); if ("fix".equals(checkIndexOnStartup)) { deprecationLogger.deprecated("Setting [index.shard.check_on_startup] is set to deprecated value [fix], " - + "which will be unsupported in future"); + + "which has no effect and will not be accepted in future"); } this.translogConfig = new TranslogConfig(shardId, shardPath().resolveTranslog(), indexSettings, bigArrays); final String aId = shardRouting.allocationId().getId(); 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 12f7b85bd185e..53d4c4471fa31 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -2785,7 +2785,7 @@ public void testCheckOnStartupDeprecatedValue() throws Exception { closeShards(newShard); assertWarnings("Setting [index.shard.check_on_startup] is set to deprecated value [fix], " - + "which will be unsupported in future"); + + "which has no effect and will not be accepted in future"); } class Result { From fee8a5bb63d34f1f3fcf7882852997bc22966ca5 Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Tue, 28 Aug 2018 15:29:25 +0200 Subject: [PATCH 10/12] dropped `fix` to avoid deprecation warnings --- .../java/org/elasticsearch/index/shard/IndexShardTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 53d4c4471fa31..0f273e13f8353 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -2736,7 +2736,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, null, indexShard.engineFactory, indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER); From 5cee2b94f934a7530ac95cb0a93095810d738863 Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Tue, 28 Aug 2018 16:13:41 +0200 Subject: [PATCH 11/12] skip files added by Lucene's ExtrasFS --- .../test/java/org/elasticsearch/index/shard/IndexShardTests.java | 1 + 1 file changed, 1 insertion(+) 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 0f273e13f8353..de2984dcea60c 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -2705,6 +2705,7 @@ private Path corruptIndexFile(ShardPath shardPath) throws IOException { .filter(p -> { final String name = p.getFileName().toString(); return Files.isRegularFile(p) + && name.startsWith("extra") == false // Skip files added by Lucene's ExtrasFS && IndexWriter.WRITE_LOCK_NAME.equals(name) == false && name.startsWith("segments_") == false && name.endsWith(".si") == false; }) From 2a9dbeb84120590cb7a39b0aeff59301aedd6bbd Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Fri, 31 Aug 2018 09:38:46 +0200 Subject: [PATCH 12/12] resolved conflicts on Merge remote-tracking branch 'remotes/origin/master' into fix/31389_1 --- .../index/shard/IndexShardTestCase.java | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) 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 5f42cdc01f4a1..375e74f6cca3e 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 @@ -178,7 +178,17 @@ public Directory newDirectory() throws IOException { * another shard) */ protected IndexShard newShard(boolean primary) throws IOException { - return newShard(primary, Settings.EMPTY, new InternalEngineFactory()); + return newShard(primary, Settings.EMPTY); + } + + /** + * 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(final boolean primary, final Settings settings) throws IOException { + return newShard(primary, settings, new InternalEngineFactory()); } /** @@ -425,7 +435,18 @@ protected IndexShard newStartedShard(final boolean primary) throws IOException { */ protected IndexShard newStartedShard( final boolean primary, final Settings settings, final EngineFactory engineFactory) throws IOException { - IndexShard shard = newShard(primary, settings, engineFactory); + return newStartedShard(p -> newShard(p, settings, engineFactory), 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 {