From c6dd17a2c605308068d0a7ca333cb0150b6d7d46 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Thu, 14 Apr 2016 11:55:17 -0700 Subject: [PATCH] Internal: Remove threadlocal from document parser The doc parser uses a context object to store the state of parsing, namely the existing mappings, new mappings, and the parsed document. Currently this uses a threadlocal which is "reset" for each doc parsed. However, the thread local doesn't actually save anything, since resetting is constructing new objects. This change removes the thread local, which also simplifies the mapper service as it now does not need to be closeable. --- .../metadata/MetaDataIndexUpgradeService.java | 9 +- .../org/elasticsearch/index/IndexService.java | 2 +- .../index/mapper/DocumentMapper.java | 4 - .../index/mapper/DocumentParser.java | 24 +--- .../index/mapper/MapperService.java | 9 +- .../index/mapper/MapperServiceTests.java | 107 +++++++----------- 6 files changed, 50 insertions(+), 105 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java index 3101a2c04cc..cdfb5487019 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java +++ b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java @@ -127,11 +127,10 @@ public class MetaDataIndexUpgradeService extends AbstractComponent { SimilarityService similarityService = new SimilarityService(indexSettings, Collections.emptyMap()); try (AnalysisService analysisService = new FakeAnalysisService(indexSettings)) { - try (MapperService mapperService = new MapperService(indexSettings, analysisService, similarityService, mapperRegistry, () -> null)) { - for (ObjectCursor cursor : indexMetaData.getMappings().values()) { - MappingMetaData mappingMetaData = cursor.value; - mapperService.merge(mappingMetaData.type(), mappingMetaData.source(), MapperService.MergeReason.MAPPING_RECOVERY, false); - } + MapperService mapperService = new MapperService(indexSettings, analysisService, similarityService, mapperRegistry, () -> null); + for (ObjectCursor cursor : indexMetaData.getMappings().values()) { + MappingMetaData mappingMetaData = cursor.value; + mapperService.merge(mappingMetaData.type(), mappingMetaData.source(), MapperService.MergeReason.MAPPING_RECOVERY, false); } } } catch (Exception ex) { diff --git a/core/src/main/java/org/elasticsearch/index/IndexService.java b/core/src/main/java/org/elasticsearch/index/IndexService.java index 5a38a194b0a..dac757477b8 100644 --- a/core/src/main/java/org/elasticsearch/index/IndexService.java +++ b/core/src/main/java/org/elasticsearch/index/IndexService.java @@ -239,7 +239,7 @@ public final class IndexService extends AbstractIndexComponent implements IndexC } } } finally { - IOUtils.close(bitsetFilterCache, indexCache, mapperService, indexFieldData, analysisService, refreshTask, fsyncTask, + IOUtils.close(bitsetFilterCache, indexCache, indexFieldData, analysisService, refreshTask, fsyncTask, cache().getPercolatorQueryCache()); } } diff --git a/core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java index 0528541238a..3eb1e84e65a 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java @@ -353,10 +353,6 @@ public class DocumentMapper implements ToXContent { return new DocumentMapper(mapperService, updated); } - public void close() { - documentParser.close(); - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { return mapping.toXContent(builder, params); diff --git a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 70219516147..a15c6b392b8 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -19,7 +19,6 @@ package org.elasticsearch.index.mapper; -import java.io.Closeable; import java.io.IOException; import java.util.ArrayList; import java.util.Collections; @@ -28,7 +27,6 @@ import java.util.List; import org.apache.lucene.document.Field; import org.apache.lucene.index.IndexableField; -import org.apache.lucene.util.CloseableThreadLocal; import org.elasticsearch.Version; import org.elasticsearch.common.Strings; import org.elasticsearch.common.joda.FormatDateTimeFormatter; @@ -38,12 +36,12 @@ import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.mapper.core.BinaryFieldMapper; import org.elasticsearch.index.mapper.core.BooleanFieldMapper; import org.elasticsearch.index.mapper.core.DateFieldMapper; +import org.elasticsearch.index.mapper.core.KeywordFieldMapper; +import org.elasticsearch.index.mapper.core.KeywordFieldMapper.KeywordFieldType; import org.elasticsearch.index.mapper.core.LegacyDateFieldMapper; import org.elasticsearch.index.mapper.core.LegacyDoubleFieldMapper; import org.elasticsearch.index.mapper.core.LegacyFloatFieldMapper; import org.elasticsearch.index.mapper.core.LegacyIntegerFieldMapper; -import org.elasticsearch.index.mapper.core.KeywordFieldMapper; -import org.elasticsearch.index.mapper.core.KeywordFieldMapper.KeywordFieldType; import org.elasticsearch.index.mapper.core.LegacyLongFieldMapper; import org.elasticsearch.index.mapper.core.NumberFieldMapper; import org.elasticsearch.index.mapper.core.StringFieldMapper; @@ -57,14 +55,7 @@ import org.elasticsearch.index.mapper.object.ObjectMapper; import org.elasticsearch.index.mapper.object.RootObjectMapper; /** A parser for documents, given mappings from a DocumentMapper */ -final class DocumentParser implements Closeable { - - private CloseableThreadLocal cache = new CloseableThreadLocal() { - @Override - protected ParseContext.InternalParseContext initialValue() { - return new ParseContext.InternalParseContext(indexSettings.getSettings(), docMapperParser, docMapper, new ContentPath(0)); - } - }; +final class DocumentParser { private final IndexSettings indexSettings; private final DocumentMapperParser docMapperParser; @@ -81,7 +72,7 @@ final class DocumentParser implements Closeable { source.type(docMapper.type()); final Mapping mapping = docMapper.mapping(); - final ParseContext.InternalParseContext context = cache.get(); + final ParseContext.InternalParseContext context = new ParseContext.InternalParseContext(indexSettings.getSettings(), docMapperParser, docMapper, new ContentPath(0)); XContentParser parser = null; try { parser = parser(source); @@ -101,8 +92,6 @@ final class DocumentParser implements Closeable { reverseOrder(context); ParsedDocument doc = parsedDocument(source, context, createDynamicUpdate(mapping, docMapper, context.getDynamicMappers())); - // reset the context to free up memory - context.reset(null, null, null); return doc; } @@ -932,9 +921,4 @@ final class DocumentParser implements Closeable { private static ObjectMapper.Dynamic dynamicOrDefault(ObjectMapper.Dynamic dynamic) { return dynamic == null ? ObjectMapper.Dynamic.TRUE : dynamic; } - - @Override - public void close() { - cache.close(); - } } diff --git a/core/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/core/src/main/java/org/elasticsearch/index/mapper/MapperService.java index ea9462071a0..ebd8587f05b 100755 --- a/core/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -63,7 +63,7 @@ import static org.elasticsearch.common.collect.MapBuilder.newMapBuilder; /** * */ -public class MapperService extends AbstractIndexComponent implements Closeable { +public class MapperService extends AbstractIndexComponent { /** * The reason why a mapping is being merged. @@ -166,13 +166,6 @@ public class MapperService extends AbstractIndexComponent implements Closeable { } } - @Override - public void close() { - for (DocumentMapper documentMapper : mappers.values()) { - documentMapper.close(); - } - } - public boolean hasNested() { return this.hasNested; } diff --git a/core/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/core/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index d43d4872914..e4c24f51c2b 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -19,11 +19,16 @@ package org.elasticsearch.index.mapper; -import org.elasticsearch.ExceptionsHelper; -import org.elasticsearch.Version; -import org.elasticsearch.cluster.metadata.IndexMetaData; -import org.elasticsearch.common.compress.CompressedXContent; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.concurrent.ExecutionException; +import java.util.function.Function; +import org.elasticsearch.ExceptionsHelper; +import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.index.IndexService; @@ -31,40 +36,23 @@ import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.index.mapper.core.KeywordFieldMapper.KeywordFieldType; import org.elasticsearch.index.mapper.core.NumberFieldMapper.NumberFieldType; import org.elasticsearch.test.ESSingleNodeTestCase; -import org.junit.Rule; -import org.junit.rules.ExpectedException; - -import java.io.IOException; -import java.io.UncheckedIOException; -import java.util.Arrays; -import java.util.Collections; -import java.util.HashSet; -import java.util.function.Function; -import java.util.concurrent.ExecutionException; import static org.hamcrest.CoreMatchers.containsString; - -import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.instanceOf; public class MapperServiceTests extends ESSingleNodeTestCase { - @Rule - public ExpectedException expectedException = ExpectedException.none(); public void testTypeNameStartsWithIllegalDot() { - expectedException.expect(MapperParsingException.class); - expectedException.expect(hasToString(containsString("mapping type name [.test-type] must not start with a '.'"))); String index = "test-index"; String type = ".test-type"; String field = "field"; - client() - .admin() - .indices() - .prepareCreate(index) - .addMapping(type, field, "type=text") - .execute() - .actionGet(); + MapperParsingException e = expectThrows(MapperParsingException.class, () -> { + client().admin().indices().prepareCreate(index) + .addMapping(type, field, "type=text") + .execute().actionGet(); + }); + assertTrue(e.getMessage(), e.getMessage().contains("mapping type name [.test-type] must not start with a '.'")); } public void testTypeNameTooLong() { @@ -72,15 +60,12 @@ public class MapperServiceTests extends ESSingleNodeTestCase { String field = "field"; String type = new String(new char[256]).replace("\0", "a"); - expectedException.expect(MapperParsingException.class); - expectedException.expect(hasToString(containsString("mapping type name [" + type + "] is too long; limit is length 255 but was [256]"))); - client() - .admin() - .indices() - .prepareCreate(index) - .addMapping(type, field, "type=text") - .execute() - .actionGet(); + MapperParsingException e = expectThrows(MapperParsingException.class, () -> { + client().admin().indices().prepareCreate(index) + .addMapping(type, field, "type=text") + .execute().actionGet(); + }); + assertTrue(e.getMessage(), e.getMessage().contains("mapping type name [" + type + "] is too long; limit is length 255 but was [256]")); } public void testTypes() throws Exception { @@ -103,36 +88,26 @@ public class MapperServiceTests extends ESSingleNodeTestCase { public void testIndexIntoDefaultMapping() throws Throwable { // 1. test implicit index creation - try { - client().prepareIndex("index1", MapperService.DEFAULT_MAPPING, "1").setSource("{").execute().get(); - fail(); - } catch (Throwable t) { - if (t instanceof ExecutionException) { - t = t.getCause(); - } - final Throwable throwable = ExceptionsHelper.unwrapCause(t); - if (throwable instanceof IllegalArgumentException) { - assertEquals("It is forbidden to index into the default mapping [_default_]", throwable.getMessage()); - } else { - throw t; - } + ExecutionException e = expectThrows(ExecutionException.class, () -> { + client().prepareIndex("index1", MapperService.DEFAULT_MAPPING, "1").setSource("{}").execute().get(); + }); + Throwable throwable = ExceptionsHelper.unwrapCause(e.getCause()); + if (throwable instanceof IllegalArgumentException) { + assertEquals("It is forbidden to index into the default mapping [_default_]", throwable.getMessage()); + } else { + throw e; } // 2. already existing index IndexService indexService = createIndex("index2"); - try { - client().prepareIndex("index2", MapperService.DEFAULT_MAPPING, "2").setSource().execute().get(); - fail(); - } catch (Throwable t) { - if (t instanceof ExecutionException) { - t = t.getCause(); - } - final Throwable throwable = ExceptionsHelper.unwrapCause(t); - if (throwable instanceof IllegalArgumentException) { - assertEquals("It is forbidden to index into the default mapping [_default_]", throwable.getMessage()); - } else { - throw t; - } + expectThrows(ExecutionException.class, () -> { + client().prepareIndex("index1", MapperService.DEFAULT_MAPPING, "2").setSource().execute().get(); + }); + throwable = ExceptionsHelper.unwrapCause(e.getCause()); + if (throwable instanceof IllegalArgumentException) { + assertEquals("It is forbidden to index into the default mapping [_default_]", throwable.getMessage()); + } else { + throw e; } assertFalse(indexService.mapperService().hasMapping(MapperService.DEFAULT_MAPPING)); } @@ -149,13 +124,11 @@ public class MapperServiceTests extends ESSingleNodeTestCase { }; createIndex("test1").mapperService().merge("type", new CompressedXContent(mapping.apply("type")), MergeReason.MAPPING_UPDATE, false); //set total number of fields to 1 to trigger an exception - try { + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> { createIndex("test2", Settings.builder().put(MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(), 1).build()) .mapperService().merge("type", new CompressedXContent(mapping.apply("type")), MergeReason.MAPPING_UPDATE, false); - fail("Expected IllegalArgumentException"); - } catch (IllegalArgumentException e) { - assertThat(e.getMessage(), containsString("Limit of total fields [1] in index [test2] has been exceeded")); - } + }); + assertTrue(e.getMessage(), e.getMessage().contains("Limit of total fields [1] in index [test2] has been exceeded")); } public void testMappingDepthExceedsLimit() throws Throwable {