From bc3136a3453a45abec66da690304d6ec694bfe8c Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Tue, 28 Apr 2015 16:05:22 +0200 Subject: [PATCH] [TEST] Cleanup Direcotry and Searcher mock code We deployed our own code to check if directories are closed etc an d if serachers are still open. Yet, since we don't have a global cluster anymore we can just use lucene's internal mechanism to do that. This commit removes all special handling and usese LuceneTestCase.closeAfterSuite to fail if certain resources are not closed Closes #10853 --- .../OldIndexBackwardsCompatibilityTests.java | 1 - .../gateway/MetaDataStateFormatTest.java | 27 --- .../gateway/RecoveryFromGatewayTests.java | 3 +- .../indices/leaks/IndicesLeaksTests.java | 3 - .../indices/recovery/IndexRecoveryTests.java | 5 +- .../SearchWithRandomExceptionsTests.java | 13 +- .../test/ElasticsearchTestCase.java | 22 -- .../org/elasticsearch/test/TestCluster.java | 2 - .../test/engine/AssertingSearcher.java | 53 +++-- .../test/engine/MockEngineSupport.java | 43 ++-- .../test/engine/MockInternalEngine.java | 10 +- .../test/engine/MockShadowEngine.java | 23 +- .../hamcrest/ElasticsearchAssertions.java | 66 ------ .../test/store/MockDirectoryHelper.java | 199 ------------------ .../test/store/MockFSDirectoryService.java | 168 +++++++++++++-- 15 files changed, 222 insertions(+), 416 deletions(-) delete mode 100644 src/test/java/org/elasticsearch/test/store/MockDirectoryHelper.java diff --git a/src/test/java/org/elasticsearch/bwcompat/OldIndexBackwardsCompatibilityTests.java b/src/test/java/org/elasticsearch/bwcompat/OldIndexBackwardsCompatibilityTests.java index 8ed550f1a98..2ff82d9f464 100644 --- a/src/test/java/org/elasticsearch/bwcompat/OldIndexBackwardsCompatibilityTests.java +++ b/src/test/java/org/elasticsearch/bwcompat/OldIndexBackwardsCompatibilityTests.java @@ -227,7 +227,6 @@ public class OldIndexBackwardsCompatibilityTests extends ElasticsearchIntegratio void unloadIndex(String indexName) throws Exception { ElasticsearchAssertions.assertAcked(client().admin().indices().prepareDelete(indexName).get()); - ElasticsearchAssertions.assertAllFilesClosed(); } public void testAllVersionsTested() throws Exception { diff --git a/src/test/java/org/elasticsearch/gateway/MetaDataStateFormatTest.java b/src/test/java/org/elasticsearch/gateway/MetaDataStateFormatTest.java index 5581290d0e5..12b699a989b 100644 --- a/src/test/java/org/elasticsearch/gateway/MetaDataStateFormatTest.java +++ b/src/test/java/org/elasticsearch/gateway/MetaDataStateFormatTest.java @@ -538,33 +538,6 @@ public class MetaDataStateFormatTest extends ElasticsearchTestCase { } } - // copied from lucene - it's package private - final class CloseableDirectory implements Closeable { - private final BaseDirectoryWrapper dir; - private final TestRuleMarkFailure failureMarker; - - public CloseableDirectory(BaseDirectoryWrapper dir, - TestRuleMarkFailure failureMarker) { - this.dir = dir; - this.failureMarker = failureMarker; - } - - @Override - public void close() throws IOException { - // We only attempt to check open/closed state if there were no other test - // failures. - try { - if (failureMarker.wasSuccessful() && dir.isOpen()) { - Assert.fail("Directory not closed: " + dir); - } - } finally { - if (dir.isOpen()) { - dir.close(); - } - } - } - } - public Path[] content(String glob, Path dir) throws IOException { try (DirectoryStream stream = Files.newDirectoryStream(dir, glob)) { return Iterators.toArray(stream.iterator(), Path.class); diff --git a/src/test/java/org/elasticsearch/gateway/RecoveryFromGatewayTests.java b/src/test/java/org/elasticsearch/gateway/RecoveryFromGatewayTests.java index 42b8822a980..44321fad582 100644 --- a/src/test/java/org/elasticsearch/gateway/RecoveryFromGatewayTests.java +++ b/src/test/java/org/elasticsearch/gateway/RecoveryFromGatewayTests.java @@ -35,7 +35,6 @@ import org.elasticsearch.indices.recovery.RecoveryState; import org.elasticsearch.test.ElasticsearchIntegrationTest; import org.elasticsearch.test.ElasticsearchIntegrationTest.ClusterScope; import org.elasticsearch.test.InternalTestCluster.RestartCallback; -import org.elasticsearch.test.store.MockDirectoryHelper; import org.elasticsearch.test.store.MockFSDirectoryService; import org.junit.Test; @@ -353,7 +352,7 @@ public class RecoveryFromGatewayTests extends ElasticsearchIntegrationTest { .put(MockFSDirectoryService.CHECK_INDEX_ON_CLOSE, false) .put("gateway.recover_after_nodes", 4) .put(ThrottlingAllocationDecider.CLUSTER_ROUTING_ALLOCATION_CONCURRENT_RECOVERIES, 4) - .put(MockDirectoryHelper.CRASH_INDEX, false).build(); + .put(MockFSDirectoryService.CRASH_INDEX, false).build(); internalCluster().startNodesAsync(4, settings).get(); // prevent any rebalance actions during the peer recovery diff --git a/src/test/java/org/elasticsearch/indices/leaks/IndicesLeaksTests.java b/src/test/java/org/elasticsearch/indices/leaks/IndicesLeaksTests.java index 28bcd9f63d9..ca0b1a52029 100644 --- a/src/test/java/org/elasticsearch/indices/leaks/IndicesLeaksTests.java +++ b/src/test/java/org/elasticsearch/indices/leaks/IndicesLeaksTests.java @@ -18,7 +18,6 @@ */ package org.elasticsearch.indices.leaks; -import org.apache.lucene.util.LuceneTestCase.BadApple; import org.elasticsearch.common.inject.Injector; import org.elasticsearch.common.settings.ImmutableSettings; import org.elasticsearch.index.mapper.DocumentMapper; @@ -28,7 +27,6 @@ import org.elasticsearch.index.shard.IndexShard; import org.elasticsearch.indices.IndicesService; import org.elasticsearch.test.ElasticsearchIntegrationTest; import org.elasticsearch.test.ElasticsearchIntegrationTest.ClusterScope; -import org.elasticsearch.test.store.MockDirectoryHelper; import org.junit.Test; import java.lang.ref.WeakReference; @@ -92,7 +90,6 @@ public class IndicesLeaksTests extends ElasticsearchIntegrationTest { shardInjector = null; cluster().wipeIndices("test"); - MockDirectoryHelper.wrappers.clear(); // we need to clear this to allow the objects to recycle for (int i = 0; i < 100; i++) { System.gc(); diff --git a/src/test/java/org/elasticsearch/indices/recovery/IndexRecoveryTests.java b/src/test/java/org/elasticsearch/indices/recovery/IndexRecoveryTests.java index 113ac6d80b7..842a6c533dc 100644 --- a/src/test/java/org/elasticsearch/indices/recovery/IndexRecoveryTests.java +++ b/src/test/java/org/elasticsearch/indices/recovery/IndexRecoveryTests.java @@ -19,7 +19,6 @@ package org.elasticsearch.indices.recovery; -import com.carrotsearch.randomizedtesting.LifecycleScope; import com.google.common.util.concurrent.ListenableFuture; import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.admin.cluster.node.stats.NodeStats; @@ -52,7 +51,7 @@ import org.elasticsearch.snapshots.SnapshotState; import org.elasticsearch.test.ElasticsearchIntegrationTest; import org.elasticsearch.test.ElasticsearchIntegrationTest.ClusterScope; import org.elasticsearch.test.junit.annotations.TestLogging; -import org.elasticsearch.test.store.MockDirectoryHelper; +import org.elasticsearch.test.store.MockFSDirectoryService; import org.elasticsearch.test.transport.MockTransportService; import org.elasticsearch.transport.*; import org.junit.Test; @@ -522,7 +521,7 @@ public class IndexRecoveryTests extends ElasticsearchIntegrationTest { .put(RecoverySettings.INDICES_RECOVERY_INTERNAL_ACTION_TIMEOUT, "1s") .put("cluster.routing.schedule", "100ms") // aggressive reroute post shard failures .put(TransportModule.TRANSPORT_SERVICE_TYPE_KEY, MockTransportService.class.getName()) - .put(MockDirectoryHelper.RANDOM_PREVENT_DOUBLE_WRITE, false) // restarted recoveries will delete temp files and write them again + .put(MockFSDirectoryService.RANDOM_PREVENT_DOUBLE_WRITE, false) // restarted recoveries will delete temp files and write them again .build(); // start a master node internalCluster().startNode(nodeSettings); diff --git a/src/test/java/org/elasticsearch/search/basic/SearchWithRandomExceptionsTests.java b/src/test/java/org/elasticsearch/search/basic/SearchWithRandomExceptionsTests.java index b4c26527639..b4fef4f623d 100644 --- a/src/test/java/org/elasticsearch/search/basic/SearchWithRandomExceptionsTests.java +++ b/src/test/java/org/elasticsearch/search/basic/SearchWithRandomExceptionsTests.java @@ -41,7 +41,6 @@ import org.elasticsearch.test.ElasticsearchIntegrationTest; import org.elasticsearch.test.engine.MockEngineSupport; import org.elasticsearch.test.engine.ThrowingLeafReaderWrapper; import org.elasticsearch.test.junit.annotations.TestLogging; -import org.elasticsearch.test.store.MockDirectoryHelper; import org.elasticsearch.test.store.MockFSDirectoryService; import org.junit.Test; @@ -108,15 +107,15 @@ public class SearchWithRandomExceptionsTests extends ElasticsearchIntegrationTes client().admin().indices().prepareFlush("test").setWaitIfOngoing(true).execute().get(); client().admin().indices().prepareClose("test").execute().get(); client().admin().indices().prepareUpdateSettings("test").setSettings(settingsBuilder() - .put(MockDirectoryHelper.RANDOM_IO_EXCEPTION_RATE, exceptionRate) - .put(MockDirectoryHelper.RANDOM_IO_EXCEPTION_RATE_ON_OPEN, exceptionOnOpenRate)); + .put(MockFSDirectoryService.RANDOM_IO_EXCEPTION_RATE, exceptionRate) + .put(MockFSDirectoryService.RANDOM_IO_EXCEPTION_RATE_ON_OPEN, exceptionOnOpenRate)); client().admin().indices().prepareOpen("test").execute().get(); } else { Builder settings = settingsBuilder() .put("index.number_of_replicas", randomIntBetween(0, 1)) .put(MockFSDirectoryService.CHECK_INDEX_ON_CLOSE, false) - .put(MockDirectoryHelper.RANDOM_IO_EXCEPTION_RATE, exceptionRate) - .put(MockDirectoryHelper.RANDOM_IO_EXCEPTION_RATE_ON_OPEN, exceptionOnOpenRate); // we cannot expect that the index will be valid + .put(MockFSDirectoryService.RANDOM_IO_EXCEPTION_RATE, exceptionRate) + .put(MockFSDirectoryService.RANDOM_IO_EXCEPTION_RATE_ON_OPEN, exceptionOnOpenRate); // we cannot expect that the index will be valid logger.info("creating index: [test] using settings: [{}]", settings.build().getAsMap()); client().admin().indices().prepareCreate("test") .setSettings(settings) @@ -190,8 +189,8 @@ public class SearchWithRandomExceptionsTests extends ElasticsearchIntegrationTes // check the index still contains the records that we indexed without errors client().admin().indices().prepareClose("test").execute().get(); client().admin().indices().prepareUpdateSettings("test").setSettings(settingsBuilder() - .put(MockDirectoryHelper.RANDOM_IO_EXCEPTION_RATE, 0) - .put(MockDirectoryHelper.RANDOM_IO_EXCEPTION_RATE_ON_OPEN, 0)); + .put(MockFSDirectoryService.RANDOM_IO_EXCEPTION_RATE, 0) + .put(MockFSDirectoryService.RANDOM_IO_EXCEPTION_RATE_ON_OPEN, 0)); client().admin().indices().prepareOpen("test").execute().get(); ensureGreen(); SearchResponse searchResponse = client().prepareSearch().setTypes("type").setQuery(QueryBuilders.matchQuery("test", "init")).get(); diff --git a/src/test/java/org/elasticsearch/test/ElasticsearchTestCase.java b/src/test/java/org/elasticsearch/test/ElasticsearchTestCase.java index 55b4b15af01..1276089b182 100644 --- a/src/test/java/org/elasticsearch/test/ElasticsearchTestCase.java +++ b/src/test/java/org/elasticsearch/test/ElasticsearchTestCase.java @@ -71,8 +71,6 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAllFilesClosed; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAllSearchersClosed; /** * Base testcase for randomized unit testing with Elasticsearch @@ -205,26 +203,6 @@ public abstract class ElasticsearchTestCase extends LuceneTestCase { System.clearProperty(EsExecutors.DEFAULT_SYSPROP); } - // check some things (like MockDirectoryWrappers) are closed where we currently - // manage them. TODO: can we add these to LuceneTestCase.closeAfterSuite directly? - // or something else simpler instead of the fake closeables? - - @BeforeClass - public static void setAfterSuiteAssertions() throws Exception { - closeAfterSuite(new Closeable() { - @Override - public void close() throws IOException { - assertAllFilesClosed(); - } - }); - closeAfterSuite(new Closeable() { - @Override - public void close() throws IOException { - assertAllSearchersClosed(); - } - }); - } - @After public final void ensureCleanedUp() throws Exception { MockPageCacheRecycler.ensureAllPagesAreReleased(); diff --git a/src/test/java/org/elasticsearch/test/TestCluster.java b/src/test/java/org/elasticsearch/test/TestCluster.java index adfa32ac1c6..d96dd084b3d 100644 --- a/src/test/java/org/elasticsearch/test/TestCluster.java +++ b/src/test/java/org/elasticsearch/test/TestCluster.java @@ -81,8 +81,6 @@ public abstract class TestCluster implements Iterable, Closeable { * This method checks all the things that need to be checked after each test */ public void assertAfterTest() throws IOException { - assertAllSearchersClosed(); - assertAllFilesClosed(); ensureEstimatedStats(); } diff --git a/src/test/java/org/elasticsearch/test/engine/AssertingSearcher.java b/src/test/java/org/elasticsearch/test/engine/AssertingSearcher.java index b3d893c4362..e88881faae9 100644 --- a/src/test/java/org/elasticsearch/test/engine/AssertingSearcher.java +++ b/src/test/java/org/elasticsearch/test/engine/AssertingSearcher.java @@ -21,17 +21,21 @@ package org.elasticsearch.test.engine; import org.apache.lucene.index.IndexReader; import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.util.LuceneTestCase; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.logging.ESLogger; import org.elasticsearch.index.engine.Engine; import org.elasticsearch.index.shard.ShardId; +import java.io.Closeable; +import java.io.IOException; import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; /** * A searcher that asserts the IndexReader's refcount on close */ -public class AssertingSearcher extends Engine.Searcher { +class AssertingSearcher extends Engine.Searcher { private final Engine.Searcher wrappedSearcher; private final ShardId shardId; private final IndexSearcher indexSearcher; @@ -39,10 +43,10 @@ public class AssertingSearcher extends Engine.Searcher { private final Object lock = new Object(); private final int initialRefCount; private final ESLogger logger; - private final Map inFlightSearchers; + private final AtomicBoolean closed = new AtomicBoolean(false); - public AssertingSearcher(IndexSearcher indexSearcher, Engine.Searcher wrappedSearcher, - ShardId shardId, Map inFlightSearchers, + AssertingSearcher(IndexSearcher indexSearcher, final Engine.Searcher wrappedSearcher, + ShardId shardId, ESLogger logger) { super(wrappedSearcher.source(), indexSearcher); // we only use the given index searcher here instead of the IS of the wrapped searcher. the IS might be a wrapped searcher @@ -53,8 +57,15 @@ public class AssertingSearcher extends Engine.Searcher { initialRefCount = wrappedSearcher.reader().getRefCount(); this.indexSearcher = indexSearcher; assert initialRefCount > 0 : "IndexReader#getRefCount() was [" + initialRefCount + "] expected a value > [0] - reader is already closed"; - this.inFlightSearchers = inFlightSearchers; - this.inFlightSearchers.put(this, new RuntimeException("Unreleased Searcher, source [" + wrappedSearcher.source() + "]")); + final RuntimeException ex = new RuntimeException("Unreleased Searcher, source [" + wrappedSearcher.source() + "]"); + LuceneTestCase.closeAfterSuite(new Closeable() { + @Override + public void close() throws IOException { + if (closed.get() == false) { + throw ex; + } + } + }); } @Override @@ -64,29 +75,25 @@ public class AssertingSearcher extends Engine.Searcher { @Override public void close() throws ElasticsearchException { - RuntimeException remove = inFlightSearchers.remove(this); synchronized (lock) { - // make sure we only get this once and store the stack of the first caller! - if (remove == null) { - assert firstReleaseStack != null; + if (closed.compareAndSet(false, true)) { + firstReleaseStack = new RuntimeException(); + final int refCount = wrappedSearcher.reader().getRefCount(); + // this assert seems to be paranoid but given LUCENE-5362 we better add some assertions here to make sure we catch any potential + // problems. + assert refCount > 0 : "IndexReader#getRefCount() was [" + refCount + "] expected a value > [0] - reader is already closed. Initial refCount was: [" + initialRefCount + "]"; + try { + wrappedSearcher.close(); + } catch (RuntimeException ex) { + logger.debug("Failed to release searcher", ex); + throw ex; + } + } else { AssertionError error = new AssertionError("Released Searcher more than once, source [" + wrappedSearcher.source() + "]"); error.initCause(firstReleaseStack); throw error; - } else { - assert firstReleaseStack == null; - firstReleaseStack = new RuntimeException("Searcher Released first here, source [" + wrappedSearcher.source() + "]"); } } - final int refCount = wrappedSearcher.reader().getRefCount(); - // this assert seems to be paranoid but given LUCENE-5362 we better add some assertions here to make sure we catch any potential - // problems. - assert refCount > 0 : "IndexReader#getRefCount() was [" + refCount + "] expected a value > [0] - reader is already closed. Initial refCount was: [" + initialRefCount + "]"; - try { - wrappedSearcher.close(); - } catch (RuntimeException ex) { - logger.debug("Failed to release searcher", ex); - throw ex; - } } @Override diff --git a/src/test/java/org/elasticsearch/test/engine/MockEngineSupport.java b/src/test/java/org/elasticsearch/test/engine/MockEngineSupport.java index 1c3581d2ae5..47c9472d2d6 100644 --- a/src/test/java/org/elasticsearch/test/engine/MockEngineSupport.java +++ b/src/test/java/org/elasticsearch/test/engine/MockEngineSupport.java @@ -33,8 +33,10 @@ import org.elasticsearch.index.engine.Engine; import org.elasticsearch.index.engine.EngineConfig; import org.elasticsearch.index.engine.EngineException; import org.elasticsearch.index.engine.InternalEngine; +import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.test.ElasticsearchIntegrationTest; +import java.io.Closeable; import java.io.IOException; import java.lang.reflect.Constructor; import java.util.Map; @@ -54,6 +56,7 @@ public final class MockEngineSupport { public static final String FLUSH_ON_CLOSE_RATIO = "index.engine.mock.flush_on_close.ratio"; private final AtomicBoolean closing = new AtomicBoolean(false); private final ESLogger logger = Loggers.getLogger(Engine.class); + private final ShardId shardId; public static class MockContext { public final Random random; @@ -71,12 +74,12 @@ public final class MockEngineSupport { } } - public static final ConcurrentMap INFLIGHT_ENGINE_SEARCHERS = new ConcurrentHashMap<>(); private final MockContext mockContext; public MockEngineSupport(EngineConfig config) { Settings indexSettings = config.getIndexSettings(); + shardId = config.getShardId(); final long seed = indexSettings.getAsLong(ElasticsearchIntegrationTest.SETTING_INDEX_SEED, 0l); Random random = new Random(seed); final double ratio = indexSettings.getAsDouble(WRAP_READER_RATIO, 0.0d); // DISABLED by default - AssertingDR is crazy slow @@ -99,33 +102,23 @@ public final class MockEngineSupport { * the first call and treats subsequent calls as if the engine passed is already closed. */ public CloseAction flushOrClose(Engine engine, CloseAction originalAction) throws IOException { - try { - if (closing.compareAndSet(false, true)) { // only do the random thing if we are the first call to this since super.flushOnClose() calls #close() again and then we might end up with a stackoverflow. - if (mockContext.flushOnClose > mockContext.random.nextDouble()) { - return CloseAction.FLUSH_AND_CLOSE; - } else { - return CloseAction.CLOSE; - } + if (closing.compareAndSet(false, true)) { // only do the random thing if we are the first call to this since super.flushOnClose() calls #close() again and then we might end up with a stackoverflow. + if (mockContext.flushOnClose > mockContext.random.nextDouble()) { + return CloseAction.FLUSH_AND_CLOSE; } else { - return originalAction; - } - } finally { - if (logger.isTraceEnabled()) { - // log debug if we have pending searchers - for (Map.Entry entry : INFLIGHT_ENGINE_SEARCHERS.entrySet()) { - logger.trace("Unreleased Searchers instance for shard [{}]", - entry.getValue(), entry.getKey().shardId()); - } + return CloseAction.CLOSE; } + } else { + return originalAction; } } - public AssertingIndexSearcher newSearcher(Engine engine, String source, IndexSearcher searcher, SearcherManager manager) throws EngineException { + public AssertingIndexSearcher newSearcher(String source, IndexSearcher searcher, SearcherManager manager) throws EngineException { IndexReader reader = searcher.getIndexReader(); IndexReader wrappedReader = reader; assert reader != null; if (reader instanceof DirectoryReader && mockContext.wrapReader) { - wrappedReader = wrapReader((DirectoryReader) reader, engine); + wrappedReader = wrapReader((DirectoryReader) reader); } // this executes basic query checks and asserts that weights are normalized only once etc. final AssertingIndexSearcher assertingIndexSearcher = new AssertingIndexSearcher(mockContext.random, wrappedReader); @@ -133,7 +126,7 @@ public final class MockEngineSupport { return assertingIndexSearcher; } - private DirectoryReader wrapReader(DirectoryReader reader, Engine engine) { + private DirectoryReader wrapReader(DirectoryReader reader) { try { Constructor[] constructors = mockContext.wrapper.getConstructors(); Constructor nonRandom = null; @@ -177,4 +170,14 @@ public final class MockEngineSupport { } + public Engine.Searcher wrapSearcher(String source, Engine.Searcher engineSearcher, IndexSearcher searcher, SearcherManager manager) { + final AssertingIndexSearcher assertingIndexSearcher = newSearcher(source, searcher, manager); + assertingIndexSearcher.setSimilarity(searcher.getSimilarity()); + // pass the original searcher to the super.newSearcher() method to make sure this is the searcher that will + // be released later on. If we wrap an index reader here must not pass the wrapped version to the manager + // on release otherwise the reader will be closed too early. - good news, stuff will fail all over the place if we don't get this right here + return new AssertingSearcher(assertingIndexSearcher, engineSearcher, shardId, logger); + } + + } diff --git a/src/test/java/org/elasticsearch/test/engine/MockInternalEngine.java b/src/test/java/org/elasticsearch/test/engine/MockInternalEngine.java index ed08a95e86f..e81009c4979 100644 --- a/src/test/java/org/elasticsearch/test/engine/MockInternalEngine.java +++ b/src/test/java/org/elasticsearch/test/engine/MockInternalEngine.java @@ -21,6 +21,7 @@ package org.elasticsearch.test.engine; import org.apache.lucene.search.AssertingIndexSearcher; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.SearcherManager; +import org.elasticsearch.index.engine.Engine; import org.elasticsearch.index.engine.EngineConfig; import org.elasticsearch.index.engine.EngineException; import org.elasticsearch.index.engine.InternalEngine; @@ -71,12 +72,7 @@ final class MockInternalEngine extends InternalEngine { @Override protected Searcher newSearcher(String source, IndexSearcher searcher, SearcherManager manager) throws EngineException { - final AssertingIndexSearcher assertingIndexSearcher = support().newSearcher(this, source, searcher, manager); - assertingIndexSearcher.setSimilarity(searcher.getSimilarity()); - // pass the original searcher to the super.newSearcher() method to make sure this is the searcher that will - // be released later on. If we wrap an index reader here must not pass the wrapped version to the manager - // on release otherwise the reader will be closed too early. - good news, stuff will fail all over the place if we don't get this right here - return new AssertingSearcher(assertingIndexSearcher, - super.newSearcher(source, searcher, manager), shardId, MockEngineSupport.INFLIGHT_ENGINE_SEARCHERS, logger); + final Searcher engineSearcher = super.newSearcher(source, searcher, manager); + return support().wrapSearcher(source, engineSearcher, searcher, manager); } } diff --git a/src/test/java/org/elasticsearch/test/engine/MockShadowEngine.java b/src/test/java/org/elasticsearch/test/engine/MockShadowEngine.java index 198d4ba6639..1ed920b20fc 100644 --- a/src/test/java/org/elasticsearch/test/engine/MockShadowEngine.java +++ b/src/test/java/org/elasticsearch/test/engine/MockShadowEngine.java @@ -37,29 +37,10 @@ final class MockShadowEngine extends ShadowEngine { this.support = new MockEngineSupport(config); } - @Override - public void close() throws IOException { - try { - super.close(); - } finally { - if (logger.isTraceEnabled()) { - // log debug if we have pending searchers - for (Map.Entry entry : MockEngineSupport.INFLIGHT_ENGINE_SEARCHERS.entrySet()) { - logger.trace("Unreleased Searchers instance for shard [{}]", entry.getValue(), entry.getKey().shardId()); - } - } - } - } - @Override protected Searcher newSearcher(String source, IndexSearcher searcher, SearcherManager manager) throws EngineException { - final AssertingIndexSearcher assertingIndexSearcher = support.newSearcher(this, source, searcher, manager); - assertingIndexSearcher.setSimilarity(searcher.getSimilarity()); - // pass the original searcher to the super.newSearcher() method to make sure this is the searcher that will - // be released later on. If we wrap an index reader here must not pass the wrapped version to the manager - // on release otherwise the reader will be closed too early. - good news, stuff will fail all over the place if we don't get this right here - return new AssertingSearcher(assertingIndexSearcher, - super.newSearcher(source, searcher, manager), shardId, MockEngineSupport.INFLIGHT_ENGINE_SEARCHERS, logger); + final Searcher engineSearcher = super.newSearcher(source, searcher, manager); + return support.wrapSearcher(source, engineSearcher, searcher, manager); } } diff --git a/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java b/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java index 0a52c36ce32..7a822e163cb 100644 --- a/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java +++ b/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java @@ -26,7 +26,6 @@ import com.google.common.collect.Iterables; import org.apache.lucene.search.BooleanQuery; import org.apache.lucene.search.Query; import org.elasticsearch.ElasticsearchException; -import org.elasticsearch.ElasticsearchIllegalStateException; import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.Version; import org.elasticsearch.action.ActionFuture; @@ -67,9 +66,6 @@ import org.elasticsearch.rest.RestStatus; import org.elasticsearch.search.SearchHit; import org.elasticsearch.search.suggest.Suggest; import org.elasticsearch.test.VersionUtils; -import org.elasticsearch.test.engine.AssertingSearcher; -import org.elasticsearch.test.engine.MockEngineSupport; -import org.elasticsearch.test.store.MockDirectoryHelper; import org.hamcrest.Matcher; import org.hamcrest.Matchers; import org.junit.Assert; @@ -80,7 +76,6 @@ import java.lang.reflect.InvocationTargetException; import java.nio.file.Files; import java.nio.file.Path; import java.util.*; -import java.util.concurrent.TimeUnit; import static com.google.common.base.Predicates.isNull; import static org.elasticsearch.test.ElasticsearchTestCase.*; @@ -683,67 +678,6 @@ public class ElasticsearchAssertions { return response; } - public static void assertAllSearchersClosed() { - /* in some cases we finish a test faster than the freeContext calls make it to the - * shards. Let's wait for some time if there are still searchers. If the are really - * pending we will fail anyway.*/ - try { - if (awaitBusy(new Predicate() { - @Override - public boolean apply(Object o) { - return MockEngineSupport.INFLIGHT_ENGINE_SEARCHERS.isEmpty(); - } - }, 5, TimeUnit.SECONDS)) { - return; - } - } catch (InterruptedException ex) { - if (MockEngineSupport.INFLIGHT_ENGINE_SEARCHERS.isEmpty()) { - return; - } - } - try { - RuntimeException ex = null; - StringBuilder builder = new StringBuilder("Unclosed Searchers instance for shards: ["); - for (Map.Entry entry : MockEngineSupport.INFLIGHT_ENGINE_SEARCHERS.entrySet()) { - ex = entry.getValue(); - builder.append(entry.getKey().shardId()).append(","); - } - builder.append("]"); - throw new RuntimeException(builder.toString(), ex); - } finally { - MockEngineSupport.INFLIGHT_ENGINE_SEARCHERS.clear(); - } - } - - public static void assertAllFilesClosed() { - try { - for (final MockDirectoryHelper.ElasticsearchMockDirectoryWrapper w : MockDirectoryHelper.wrappers) { - try { - w.awaitClosed(5000); - } catch (InterruptedException e) { - Thread.interrupted(); - } - if (!w.successfullyClosed()) { - if (w.closeException() == null) { - try { - w.close(); - } catch (IOException e) { - throw new ElasticsearchIllegalStateException("directory close threw IOException", e); - } - if (w.closeException() != null) { - throw w.closeException(); - } - } else { - throw w.closeException(); - } - } - assertThat(w.isOpen(), is(false)); - } - } finally { - MockDirectoryHelper.wrappers.clear(); - } - } - public static void assertNodeContainsPlugins(NodesInfoResponse response, String nodeId, List expectedJvmPluginNames, List expectedJvmPluginDescriptions, diff --git a/src/test/java/org/elasticsearch/test/store/MockDirectoryHelper.java b/src/test/java/org/elasticsearch/test/store/MockDirectoryHelper.java deleted file mode 100644 index 90e06d6a5df..00000000000 --- a/src/test/java/org/elasticsearch/test/store/MockDirectoryHelper.java +++ /dev/null @@ -1,199 +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.test.store; - -import com.carrotsearch.randomizedtesting.generators.RandomPicks; -import org.apache.lucene.store.Directory; -import org.apache.lucene.store.FilterDirectory; -import org.apache.lucene.store.MockDirectoryWrapper.Throttling; -import org.apache.lucene.store.MockDirectoryWrapper; -import org.apache.lucene.store.NRTCachingDirectory; -import org.elasticsearch.common.logging.ESLogger; -import org.elasticsearch.common.settings.ImmutableSettings; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.util.concurrent.ConcurrentCollections; -import org.elasticsearch.index.shard.ShardId; -import org.elasticsearch.index.shard.ShardPath; -import org.elasticsearch.index.store.FsDirectoryService; -import org.elasticsearch.index.store.IndexStore; -import org.elasticsearch.index.store.IndexStoreModule; -import com.carrotsearch.randomizedtesting.SeedUtils; - -import java.io.IOException; -import java.lang.reflect.Field; -import java.util.Collection; -import java.util.Random; -import java.util.Set; - -public class MockDirectoryHelper { - public static final String RANDOM_IO_EXCEPTION_RATE = "index.store.mock.random.io_exception_rate"; - public static final String RANDOM_IO_EXCEPTION_RATE_ON_OPEN = "index.store.mock.random.io_exception_rate_on_open"; - public static final String RANDOM_PREVENT_DOUBLE_WRITE = "index.store.mock.random.prevent_double_write"; - public static final String RANDOM_NO_DELETE_OPEN_FILE = "index.store.mock.random.no_delete_open_file"; - public static final String CRASH_INDEX = "index.store.mock.random.crash_index"; - - public static final Set wrappers = ConcurrentCollections.newConcurrentSet(); - - private final Random random; - private final double randomIOExceptionRate; - private final double randomIOExceptionRateOnOpen; - private final Throttling throttle; - private final Settings indexSettings; - private final ShardId shardId; - private final boolean preventDoubleWrite; - private final boolean noDeleteOpenFile; - private final ESLogger logger; - private final boolean crashIndex; - - public MockDirectoryHelper(ShardId shardId, Settings indexSettings, ESLogger logger, Random random, long seed) { - this.random = random; - randomIOExceptionRate = indexSettings.getAsDouble(RANDOM_IO_EXCEPTION_RATE, 0.0d); - randomIOExceptionRateOnOpen = indexSettings.getAsDouble(RANDOM_IO_EXCEPTION_RATE_ON_OPEN, 0.0d); - preventDoubleWrite = indexSettings.getAsBoolean(RANDOM_PREVENT_DOUBLE_WRITE, true); // true is default in MDW - noDeleteOpenFile = indexSettings.getAsBoolean(RANDOM_NO_DELETE_OPEN_FILE, random.nextBoolean()); // true is default in MDW - random.nextInt(shardId.getId() + 1); // some randomness per shard - throttle = Throttling.NEVER; - crashIndex = indexSettings.getAsBoolean(CRASH_INDEX, true); - - if (logger.isDebugEnabled()) { - logger.debug("Using MockDirWrapper with seed [{}] throttle: [{}] crashIndex: [{}]", SeedUtils.formatSeed(seed), - throttle, crashIndex); - } - this.indexSettings = indexSettings; - this.shardId = shardId; - this.logger = logger; - } - - public Directory wrap(Directory dir) { - final ElasticsearchMockDirectoryWrapper w = new ElasticsearchMockDirectoryWrapper(random, dir, logger, this.crashIndex); - w.setRandomIOExceptionRate(randomIOExceptionRate); - w.setRandomIOExceptionRateOnOpen(randomIOExceptionRateOnOpen); - w.setThrottling(throttle); - w.setCheckIndexOnClose(false); // we do this on the index level - w.setPreventDoubleWrite(preventDoubleWrite); - // TODO: make this test robust to virus scanner - w.setEnableVirusScanner(false); - w.setNoDeleteOpenFile(noDeleteOpenFile); - w.setUseSlowOpenClosers(false); - wrappers.add(w); - return w; - } - - public FsDirectoryService randomDirectorService(IndexStore indexStore, ShardPath path) { - ImmutableSettings.Builder builder = ImmutableSettings.settingsBuilder(); - builder.put(indexSettings); - builder.put(IndexStoreModule.STORE_TYPE, RandomPicks.randomFrom(random, IndexStoreModule.Type.values())); - return new FsDirectoryService(builder.build(), indexStore, path); - } - - public static final class ElasticsearchMockDirectoryWrapper extends MockDirectoryWrapper { - - private final ESLogger logger; - private final boolean crash; - private volatile RuntimeException closeException; - private final Object lock = new Object(); - private final Set superUnSyncedFiles; - private final Random superRandomState; - - public ElasticsearchMockDirectoryWrapper(Random random, Directory delegate, ESLogger logger, boolean crash) { - super(random, delegate); - this.crash = crash; - this.logger = logger; - - // TODO: remove all this and cutover to MockFS (DisableFsyncFS) instead - try { - Field field = MockDirectoryWrapper.class.getDeclaredField("unSyncedFiles"); - field.setAccessible(true); - superUnSyncedFiles = (Set) field.get(this); - - field = MockDirectoryWrapper.class.getDeclaredField("randomState"); - field.setAccessible(true); - superRandomState = (Random) field.get(this); - } catch (ReflectiveOperationException roe) { - throw new RuntimeException(roe); - } - } - - @Override - public synchronized void close() throws IOException { - try { - super.close(); - } catch (RuntimeException ex) { - logger.info("MockDirectoryWrapper#close() threw exception", ex); - closeException = ex; - throw ex; - } finally { - synchronized (lock) { - lock.notifyAll(); - } - } - } - - /** - * Returns true if {@link #in} must sync its files. - * Currently, only {@link NRTCachingDirectory} requires sync'ing its files - * because otherwise they are cached in an internal {@link org.apache.lucene.store.RAMDirectory}. If - * other directories require that too, they should be added to this method. - */ - private boolean mustSync() { - Directory delegate = in; - while (delegate instanceof FilterDirectory) { - if (delegate instanceof NRTCachingDirectory) { - return true; - } - delegate = ((FilterDirectory) delegate).getDelegate(); - } - return delegate instanceof NRTCachingDirectory; - } - - @Override - public synchronized void sync(Collection names) throws IOException { - // don't wear out our hardware so much in tests. - if (superRandomState.nextInt(100) == 0 || mustSync()) { - super.sync(names); - } else { - superUnSyncedFiles.removeAll(names); - } - } - - public void awaitClosed(long timeout) throws InterruptedException { - synchronized (lock) { - if(isOpen()) { - lock.wait(timeout); - } - } - } - - public synchronized boolean successfullyClosed() { - return closeException == null && !isOpen(); - } - - public synchronized RuntimeException closeException() { - return closeException; - } - - @Override - public synchronized void crash() throws IOException { - if (crash) { - super.crash(); - } - } - } -} diff --git a/src/test/java/org/elasticsearch/test/store/MockFSDirectoryService.java b/src/test/java/org/elasticsearch/test/store/MockFSDirectoryService.java index c10c2863db1..d07b3b7c4d5 100644 --- a/src/test/java/org/elasticsearch/test/store/MockFSDirectoryService.java +++ b/src/test/java/org/elasticsearch/test/store/MockFSDirectoryService.java @@ -19,57 +19,86 @@ package org.elasticsearch.test.store; +import com.carrotsearch.randomizedtesting.SeedUtils; +import com.carrotsearch.randomizedtesting.generators.RandomPicks; import com.google.common.base.Charsets; import org.apache.lucene.index.CheckIndex; import org.apache.lucene.index.IndexWriter; -import org.apache.lucene.store.Directory; -import org.apache.lucene.store.LockFactory; -import org.apache.lucene.store.StoreRateLimiting; +import org.apache.lucene.store.*; +import org.apache.lucene.util.LuceneTestCase; +import org.apache.lucene.util.TestRuleMarkFailure; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.lease.Releasables; import org.elasticsearch.common.lucene.Lucene; +import org.elasticsearch.common.settings.ImmutableSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.settings.IndexSettings; import org.elasticsearch.index.shard.*; import org.elasticsearch.index.store.IndexStore; +import org.elasticsearch.index.store.IndexStoreModule; import org.elasticsearch.index.store.Store; import org.elasticsearch.index.store.FsDirectoryService; import org.elasticsearch.indices.IndicesLifecycle; import org.elasticsearch.indices.IndicesService; import org.elasticsearch.test.ElasticsearchTestCase; import org.elasticsearch.test.ElasticsearchIntegrationTest; +import org.junit.Assert; +import java.io.Closeable; import java.io.IOException; import java.io.PrintStream; +import java.lang.reflect.Field; import java.nio.file.Path; -import java.util.Arrays; -import java.util.EnumSet; -import java.util.Random; +import java.util.*; public class MockFSDirectoryService extends FsDirectoryService { + public static final String CHECK_INDEX_ON_CLOSE = "index.store.mock.check_index_on_close"; + public static final String RANDOM_IO_EXCEPTION_RATE_ON_OPEN = "index.store.mock.random.io_exception_rate_on_open"; + public static final String RANDOM_PREVENT_DOUBLE_WRITE = "index.store.mock.random.prevent_double_write"; + public static final String RANDOM_NO_DELETE_OPEN_FILE = "index.store.mock.random.no_delete_open_file"; + public static final String CRASH_INDEX = "index.store.mock.random.crash_index"; + private static final EnumSet validCheckIndexStates = EnumSet.of( IndexShardState.STARTED, IndexShardState.RELOCATED , IndexShardState.POST_RECOVERY ); - private final MockDirectoryHelper helper; - private FsDirectoryService delegateService; - public static final String CHECK_INDEX_ON_CLOSE = "index.store.mock.check_index_on_close"; + private final FsDirectoryService delegateService; private final boolean checkIndexOnClose; + private final Random random; + private final double randomIOExceptionRate; + private final double randomIOExceptionRateOnOpen; + private final MockDirectoryWrapper.Throttling throttle; + private final Settings indexSettings; + private final boolean preventDoubleWrite; + private final boolean noDeleteOpenFile; + private final boolean crashIndex; @Inject public MockFSDirectoryService(@IndexSettings Settings indexSettings, IndexStore indexStore, final IndicesService service, final ShardPath path) { super(indexSettings, indexStore, path); final long seed = indexSettings.getAsLong(ElasticsearchIntegrationTest.SETTING_INDEX_SEED, 0l); - Random random = new Random(seed); - helper = new MockDirectoryHelper(shardId, indexSettings, logger, random, seed); + this.random = new Random(seed); checkIndexOnClose = indexSettings.getAsBoolean(CHECK_INDEX_ON_CLOSE, true); + randomIOExceptionRate = indexSettings.getAsDouble(RANDOM_IO_EXCEPTION_RATE, 0.0d); + randomIOExceptionRateOnOpen = indexSettings.getAsDouble(RANDOM_IO_EXCEPTION_RATE_ON_OPEN, 0.0d); + preventDoubleWrite = indexSettings.getAsBoolean(RANDOM_PREVENT_DOUBLE_WRITE, true); // true is default in MDW + noDeleteOpenFile = indexSettings.getAsBoolean(RANDOM_NO_DELETE_OPEN_FILE, random.nextBoolean()); // true is default in MDW + random.nextInt(shardId.getId() + 1); // some randomness per shard + throttle = MockDirectoryWrapper.Throttling.NEVER; + crashIndex = indexSettings.getAsBoolean(CRASH_INDEX, true); - delegateService = helper.randomDirectorService(indexStore, path); + if (logger.isDebugEnabled()) { + logger.debug("Using MockDirWrapper with seed [{}] throttle: [{}] crashIndex: [{}]", SeedUtils.formatSeed(seed), + throttle, crashIndex); + } + this.indexSettings = indexSettings; + delegateService = randomDirectorService(indexStore, path); if (checkIndexOnClose) { final IndicesLifecycle.Listener listener = new IndicesLifecycle.Listener() { @@ -112,7 +141,7 @@ public class MockFSDirectoryService extends FsDirectoryService { @Override public Directory newDirectory() throws IOException { - return helper.wrap(delegateService.newDirectory()); + return wrap(delegateService.newDirectory()); } @Override @@ -173,4 +202,117 @@ public class MockFSDirectoryService extends FsDirectoryService { public long throttleTimeInNanos() { return delegateService.throttleTimeInNanos(); } + + public static final String RANDOM_IO_EXCEPTION_RATE = "index.store.mock.random.io_exception_rate"; + + private Directory wrap(Directory dir) { + final ElasticsearchMockDirectoryWrapper w = new ElasticsearchMockDirectoryWrapper(random, dir, this.crashIndex); + w.setRandomIOExceptionRate(randomIOExceptionRate); + w.setRandomIOExceptionRateOnOpen(randomIOExceptionRateOnOpen); + w.setThrottling(throttle); + w.setCheckIndexOnClose(false); // we do this on the index level + w.setPreventDoubleWrite(preventDoubleWrite); + // TODO: make this test robust to virus scanner + w.setEnableVirusScanner(false); + w.setNoDeleteOpenFile(noDeleteOpenFile); + w.setUseSlowOpenClosers(false); + LuceneTestCase.closeAfterSuite(new CloseableDirectory(w)); + return w; + } + + private FsDirectoryService randomDirectorService(IndexStore indexStore, ShardPath path) { + ImmutableSettings.Builder builder = ImmutableSettings.settingsBuilder(); + builder.put(indexSettings); + builder.put(IndexStoreModule.STORE_TYPE, RandomPicks.randomFrom(random, IndexStoreModule.Type.values())); + return new FsDirectoryService(builder.build(), indexStore, path); + } + + public static final class ElasticsearchMockDirectoryWrapper extends MockDirectoryWrapper { + + private final boolean crash; + private final Set superUnSyncedFiles; + private final Random superRandomState; + + public ElasticsearchMockDirectoryWrapper(Random random, Directory delegate, boolean crash) { + super(random, delegate); + this.crash = crash; + + // TODO: remove all this and cutover to MockFS (DisableFsyncFS) instead + try { + Field field = MockDirectoryWrapper.class.getDeclaredField("unSyncedFiles"); + field.setAccessible(true); + superUnSyncedFiles = (Set) field.get(this); + + field = MockDirectoryWrapper.class.getDeclaredField("randomState"); + field.setAccessible(true); + superRandomState = (Random) field.get(this); + } catch (ReflectiveOperationException roe) { + throw new RuntimeException(roe); + } + } + + /** + * Returns true if {@link #in} must sync its files. + * Currently, only {@link org.apache.lucene.store.NRTCachingDirectory} requires sync'ing its files + * because otherwise they are cached in an internal {@link org.apache.lucene.store.RAMDirectory}. If + * other directories require that too, they should be added to this method. + */ + private boolean mustSync() { + Directory delegate = in; + while (delegate instanceof FilterDirectory) { + if (delegate instanceof NRTCachingDirectory) { + return true; + } + delegate = ((FilterDirectory) delegate).getDelegate(); + } + return delegate instanceof NRTCachingDirectory; + } + + @Override + public synchronized void sync(Collection names) throws IOException { + // don't wear out our hardware so much in tests. + if (superRandomState.nextInt(100) == 0 || mustSync()) { + super.sync(names); + } else { + superUnSyncedFiles.removeAll(names); + } + } + + @Override + public synchronized void crash() throws IOException { + if (crash) { + super.crash(); + } + } + } + + final class CloseableDirectory implements Closeable { + private final BaseDirectoryWrapper dir; + private final TestRuleMarkFailure failureMarker; + + public CloseableDirectory(BaseDirectoryWrapper dir) { + this.dir = dir; + try { + final Field suiteFailureMarker = LuceneTestCase.class.getDeclaredField("suiteFailureMarker"); + suiteFailureMarker.setAccessible(true); + this.failureMarker = (TestRuleMarkFailure) suiteFailureMarker.get(LuceneTestCase.class); + } catch (Throwable e) { + throw new ElasticsearchException("foo", e); + } + } + + @Override + public void close() { + // We only attempt to check open/closed state if there were no other test + // failures. + try { + if (failureMarker.wasSuccessful() && dir.isOpen()) { + Assert.fail("Directory not closed: " + dir); + } + } finally { + // TODO: perform real close of the delegate: LUCENE-4058 + // dir.close(); + } + } + } }