From 6dd843426c38de0dcde430c4160f95916712e6fb Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Thu, 7 May 2015 10:17:52 -0700 Subject: [PATCH] Mappings: Remove mapper listeners The mapper listener concept is only now used as a callback to the MapperService when new fields are added. This change removes the listeners, instead storing a link to the mapper service in each doc mapper. --- .../index/mapper/DocumentMapper.java | 49 ++++++++++--------- .../index/mapper/DocumentMapperParser.java | 6 ++- .../index/mapper/FieldMapperListener.java | 47 ------------------ .../index/mapper/MapperService.java | 35 ++----------- .../index/mapper/MapperUtils.java | 20 ++++++-- .../index/mapper/MergeResult.java | 4 ++ .../index/mapper/ObjectMapperListener.java | 46 ----------------- .../index/mapper/object/ObjectMapper.java | 2 - .../index/engine/InternalEngineTests.java | 14 +++--- .../mapper/core/BooleanFieldMapperTests.java | 2 - .../mapper/externalvalues/ExternalMapper.java | 4 +- .../mapper/multifield/MultiFieldTests.java | 2 +- .../mapper/simple/SimpleMapperTests.java | 4 +- 13 files changed, 63 insertions(+), 172 deletions(-) delete mode 100644 src/main/java/org/elasticsearch/index/mapper/FieldMapperListener.java delete mode 100644 src/main/java/org/elasticsearch/index/mapper/ObjectMapperListener.java diff --git a/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java b/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java index 09489c252c9..39da9de1a66 100644 --- a/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java @@ -158,12 +158,14 @@ public class DocumentMapper implements ToXContent { return this; } - public DocumentMapper build(DocumentMapperParser docMapperParser) { + public DocumentMapper build(MapperService mapperService, DocumentMapperParser docMapperParser) { Preconditions.checkNotNull(rootObjectMapper, "Mapper builder must have the root object mapper set"); - return new DocumentMapper(index, indexSettings, docMapperParser, rootObjectMapper, meta, rootMappers, sourceTransforms); + return new DocumentMapper(mapperService, index, indexSettings, docMapperParser, rootObjectMapper, meta, rootMappers, sourceTransforms); } } + private final MapperService mapperService; + private final String type; private final StringAndBytesText typeText; @@ -177,20 +179,17 @@ public class DocumentMapper implements ToXContent { private volatile ImmutableMap objectMappers = ImmutableMap.of(); - private final List fieldMapperListeners = new CopyOnWriteArrayList<>(); - - private final List objectMapperListeners = new CopyOnWriteArrayList<>(); - private boolean hasNestedObjects = false; private final Filter typeFilter; private final Object mappersMutex = new Object(); - public DocumentMapper(String index, @Nullable Settings indexSettings, DocumentMapperParser docMapperParser, + public DocumentMapper(MapperService mapperService, String index, @Nullable Settings indexSettings, DocumentMapperParser docMapperParser, RootObjectMapper rootObjectMapper, ImmutableMap meta, Map, RootMapper> rootMappers, List sourceTransforms) { + this.mapperService = mapperService; this.type = rootObjectMapper.name(); this.typeText = new StringAndBytesText(this.type); this.mapping = new Mapping( @@ -414,13 +413,7 @@ public class DocumentMapper implements ToXContent { synchronized (mappersMutex) { this.fieldMappers = this.fieldMappers.copyAndAllAll(fieldMappers); } - for (FieldMapperListener listener : fieldMapperListeners) { - listener.fieldMappers(fieldMappers); - } - } - - public void addFieldMapperListener(FieldMapperListener fieldMapperListener) { - fieldMapperListeners.add(fieldMapperListener); + mapperService.addFieldMappers(fieldMappers); } private void addObjectMappers(Collection objectMappers) { @@ -434,30 +427,36 @@ public class DocumentMapper implements ToXContent { } this.objectMappers = builder.immutableMap(); } - for (ObjectMapperListener objectMapperListener : objectMapperListeners) { - objectMapperListener.objectMappers(objectMappers); - } - } - - public void addObjectMapperListener(ObjectMapperListener objectMapperListener) { - objectMapperListeners.add(objectMapperListener); + mapperService.addObjectMappers(objectMappers); } private MergeResult newMergeContext(boolean simulate) { return new MergeResult(simulate) { - List conflicts = new ArrayList<>(); + final List conflicts = new ArrayList<>(); + final List> newFieldMappers = new ArrayList<>(); + final List newObjectMappers = new ArrayList<>(); @Override public void addFieldMappers(Collection> fieldMappers) { assert simulate() == false; - DocumentMapper.this.addFieldMappers(fieldMappers); + newFieldMappers.addAll(fieldMappers); } @Override public void addObjectMappers(Collection objectMappers) { assert simulate() == false; - DocumentMapper.this.addObjectMappers(objectMappers); + newObjectMappers.addAll(objectMappers); + } + + @Override + public Collection> getNewFieldMappers() { + return newFieldMappers; + } + + @Override + public Collection getNewObjectMappers() { + return newObjectMappers; } @Override @@ -482,6 +481,8 @@ public class DocumentMapper implements ToXContent { final MergeResult mergeResult = newMergeContext(simulate); this.mapping.merge(mapping, mergeResult); if (simulate == false) { + addFieldMappers(mergeResult.getNewFieldMappers()); + addObjectMappers(mergeResult.getNewObjectMappers()); refreshSource(); } return mergeResult; diff --git a/src/main/java/org/elasticsearch/index/mapper/DocumentMapperParser.java b/src/main/java/org/elasticsearch/index/mapper/DocumentMapperParser.java index 8653366788d..c955d6aeb6a 100644 --- a/src/main/java/org/elasticsearch/index/mapper/DocumentMapperParser.java +++ b/src/main/java/org/elasticsearch/index/mapper/DocumentMapperParser.java @@ -87,6 +87,7 @@ import static org.elasticsearch.index.mapper.MapperBuilders.doc; */ public class DocumentMapperParser extends AbstractIndexComponent { + final MapperService mapperService; final AnalysisService analysisService; private static final ESLogger logger = Loggers.getLogger(DocumentMapperParser.class); private final SimilarityLookupService similarityLookupService; @@ -100,9 +101,10 @@ public class DocumentMapperParser extends AbstractIndexComponent { private volatile ImmutableMap typeParsers; private volatile ImmutableMap rootTypeParsers; - public DocumentMapperParser(Index index, @IndexSettings Settings indexSettings, AnalysisService analysisService, + public DocumentMapperParser(Index index, @IndexSettings Settings indexSettings, MapperService mapperService, AnalysisService analysisService, SimilarityLookupService similarityLookupService, ScriptService scriptService) { super(index, indexSettings); + this.mapperService = mapperService; this.analysisService = analysisService; this.similarityLookupService = similarityLookupService; this.scriptService = scriptService; @@ -269,7 +271,7 @@ public class DocumentMapperParser extends AbstractIndexComponent { checkNoRemainingFields(mapping, parserContext.indexVersionCreated(), "Root mapping definition has unsupported parameters: "); - DocumentMapper documentMapper = docBuilder.build(this); + DocumentMapper documentMapper = docBuilder.build(mapperService, this); // update the source with the generated one documentMapper.refreshSource(); return documentMapper; diff --git a/src/main/java/org/elasticsearch/index/mapper/FieldMapperListener.java b/src/main/java/org/elasticsearch/index/mapper/FieldMapperListener.java deleted file mode 100644 index 3251ed5203f..00000000000 --- a/src/main/java/org/elasticsearch/index/mapper/FieldMapperListener.java +++ /dev/null @@ -1,47 +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.mapper; - -import java.util.ArrayList; -import java.util.Collection; -import java.util.List; - -/** - * - */ -public abstract class FieldMapperListener { - - public static class Aggregator extends FieldMapperListener { - public final List> mappers = new ArrayList<>(); - - @Override - public void fieldMapper(FieldMapper fieldMapper) { - mappers.add(fieldMapper); - } - } - - public abstract void fieldMapper(FieldMapper fieldMapper); - - public void fieldMappers(Collection> fieldMappers) { - for (FieldMapper mapper : fieldMappers) { - fieldMapper(mapper); - } - } -} diff --git a/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/src/main/java/org/elasticsearch/index/mapper/MapperService.java index d03d646f64e..b2a82e39648 100755 --- a/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -104,9 +104,6 @@ public class MapperService extends AbstractIndexComponent { private final DocumentMapperParser documentParser; - private final InternalFieldMapperListener fieldMapperListener = new InternalFieldMapperListener(); - private final InternalObjectMapperListener objectMapperListener = new InternalObjectMapperListener(); - private final SmartIndexNameSearchAnalyzer searchAnalyzer; private final SmartIndexNameSearchQuoteAnalyzer searchQuoteAnalyzer; @@ -122,7 +119,7 @@ public class MapperService extends AbstractIndexComponent { this.analysisService = analysisService; this.fieldDataService = fieldDataService; this.fieldMappers = new FieldMappersLookup(); - this.documentParser = new DocumentMapperParser(index, indexSettings, analysisService, similarityLookupService, scriptService); + this.documentParser = new DocumentMapperParser(index, indexSettings, this, analysisService, similarityLookupService, scriptService); this.searchAnalyzer = new SmartIndexNameSearchAnalyzer(analysisService.defaultSearchAnalyzer()); this.searchQuoteAnalyzer = new SmartIndexNameSearchQuoteAnalyzer(analysisService.defaultSearchQuoteAnalyzer()); @@ -275,9 +272,7 @@ public class MapperService extends AbstractIndexComponent { } MapperUtils.collect(mapper.mapping().root, newObjectMappers, newFieldMappers); addFieldMappers(newFieldMappers); - mapper.addFieldMapperListener(fieldMapperListener); addObjectMappers(newObjectMappers); - mapper.addObjectMapperListener(objectMapperListener); for (DocumentTypeListener typeListener : typeListeners) { typeListener.beforeCreate(mapper); @@ -288,7 +283,7 @@ public class MapperService extends AbstractIndexComponent { } } - private void addObjectMappers(Collection objectMappers) { + protected void addObjectMappers(Collection objectMappers) { synchronized (mappersMutex) { ImmutableOpenMap.Builder fullPathObjectMappers = ImmutableOpenMap.builder(this.fullPathObjectMappers); for (ObjectMapper objectMapper : objectMappers) { @@ -308,7 +303,7 @@ public class MapperService extends AbstractIndexComponent { } } - private void addFieldMappers(Collection> fieldMappers) { + protected void addFieldMappers(Collection> fieldMappers) { synchronized (mappersMutex) { this.fieldMappers = this.fieldMappers.copyAndAddAll(fieldMappers); } @@ -848,28 +843,4 @@ public class MapperService extends AbstractIndexComponent { return defaultAnalyzer; } } - - class InternalFieldMapperListener extends FieldMapperListener { - @Override - public void fieldMapper(FieldMapper fieldMapper) { - addFieldMappers(Collections.>singletonList(fieldMapper)); - } - - @Override - public void fieldMappers(Collection> fieldMappers) { - addFieldMappers(fieldMappers); - } - } - - class InternalObjectMapperListener extends ObjectMapperListener { - @Override - public void objectMapper(ObjectMapper objectMapper) { - addObjectMappers(Collections.singletonList(objectMapper)); - } - - @Override - public void objectMappers(Collection objectMappers) { - addObjectMappers(objectMappers); - } - } } diff --git a/src/main/java/org/elasticsearch/index/mapper/MapperUtils.java b/src/main/java/org/elasticsearch/index/mapper/MapperUtils.java index 74ec70828de..9ee7e052969 100644 --- a/src/main/java/org/elasticsearch/index/mapper/MapperUtils.java +++ b/src/main/java/org/elasticsearch/index/mapper/MapperUtils.java @@ -29,7 +29,7 @@ import java.util.Collection; public enum MapperUtils { ; - private static MergeResult newStrictMergeContext() { + private static MergeResult newStrictMergeResult() { return new MergeResult(false) { @Override @@ -42,14 +42,24 @@ public enum MapperUtils { return Strings.EMPTY_ARRAY; } + @Override + public void addFieldMappers(Collection> fieldMappers) { + // no-op + } + @Override public void addObjectMappers(Collection objectMappers) { // no-op } @Override - public void addFieldMappers(Collection> fieldMappers) { - // no-op + public Collection> getNewFieldMappers() { + throw new UnsupportedOperationException("Strict merge result does not support new field mappers"); + } + + @Override + public Collection getNewObjectMappers() { + throw new UnsupportedOperationException("Strict merge result does not support new object mappers"); } @Override @@ -64,7 +74,7 @@ public enum MapperUtils { * merges mappings, not lookup structures. Conflicts are returned as exceptions. */ public static void merge(Mapper mergeInto, Mapper mergeWith) { - mergeInto.merge(mergeWith, newStrictMergeContext()); + mergeInto.merge(mergeWith, newStrictMergeResult()); } /** @@ -72,7 +82,7 @@ public enum MapperUtils { * merges mappings, not lookup structures. Conflicts are returned as exceptions. */ public static void merge(Mapping mergeInto, Mapping mergeWith) { - mergeInto.merge(mergeWith, newStrictMergeContext()); + mergeInto.merge(mergeWith, newStrictMergeResult()); } /** Split mapper and its descendants into object and field mappers. */ diff --git a/src/main/java/org/elasticsearch/index/mapper/MergeResult.java b/src/main/java/org/elasticsearch/index/mapper/MergeResult.java index ab685f624ef..556491ce1f2 100644 --- a/src/main/java/org/elasticsearch/index/mapper/MergeResult.java +++ b/src/main/java/org/elasticsearch/index/mapper/MergeResult.java @@ -38,6 +38,10 @@ public abstract class MergeResult { public abstract void addObjectMappers(Collection objectMappers); + public abstract Collection> getNewFieldMappers(); + + public abstract Collection getNewObjectMappers(); + public boolean simulate() { return simulate; } diff --git a/src/main/java/org/elasticsearch/index/mapper/ObjectMapperListener.java b/src/main/java/org/elasticsearch/index/mapper/ObjectMapperListener.java deleted file mode 100644 index d6890c91917..00000000000 --- a/src/main/java/org/elasticsearch/index/mapper/ObjectMapperListener.java +++ /dev/null @@ -1,46 +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.mapper; - -import org.elasticsearch.index.mapper.object.ObjectMapper; - -import java.util.ArrayList; -import java.util.Collection; -import java.util.List; - -public abstract class ObjectMapperListener { - - public static class Aggregator extends ObjectMapperListener { - public final List mappers = new ArrayList<>(); - - @Override - public void objectMapper(ObjectMapper objectMapper) { - mappers.add(objectMapper); - } - } - - public abstract void objectMapper(ObjectMapper objectMapper); - - public void objectMappers(Collection objectMappers) { - for (ObjectMapper objectMapper : objectMappers) { - objectMapper(objectMapper); - } - } -} diff --git a/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java b/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java index dec5fca57c0..a43410dc46a 100644 --- a/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java @@ -37,14 +37,12 @@ import org.elasticsearch.index.mapper.ContentPath; import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.DocumentMapperParser; import org.elasticsearch.index.mapper.FieldMapper; -import org.elasticsearch.index.mapper.FieldMapperListener; import org.elasticsearch.index.mapper.InternalMapper; import org.elasticsearch.index.mapper.Mapper; import org.elasticsearch.index.mapper.MapperParsingException; import org.elasticsearch.index.mapper.MapperUtils; import org.elasticsearch.index.mapper.MergeMappingException; import org.elasticsearch.index.mapper.MergeResult; -import org.elasticsearch.index.mapper.ObjectMapperListener; import org.elasticsearch.index.mapper.internal.AllFieldMapper; import org.elasticsearch.index.mapper.internal.TypeFieldMapper; import org.elasticsearch.index.settings.IndexSettings; diff --git a/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java index 508805239ed..65406c910b4 100644 --- a/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java +++ b/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java @@ -1753,15 +1753,17 @@ public class InternalEngineTests extends ElasticsearchTestCase { public final AtomicInteger recoveredOps = new AtomicInteger(0); - public TranslogHandler(String index) { + public TranslogHandler(String indexName) { super(null, new MapperAnalyzer(null), null, null, null); Settings settings = ImmutableSettings.settingsBuilder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).build(); RootObjectMapper.Builder rootBuilder = new RootObjectMapper.Builder("test"); - DocumentMapper.Builder b = new DocumentMapper.Builder(index, settings, rootBuilder); - DocumentMapperParser parser = new DocumentMapperParser(new Index(index), settings, - new AnalysisService(new Index(index), settings), - new SimilarityLookupService(new Index(index), settings), null); - this.docMapper = b.build(parser); + Index index = new Index(indexName); + AnalysisService analysisService = new AnalysisService(index, settings); + SimilarityLookupService similarityLookupService = new SimilarityLookupService(index, settings); + MapperService mapperService = new MapperService(index, settings, analysisService, null, similarityLookupService, null); + DocumentMapper.Builder b = new DocumentMapper.Builder(indexName, settings, rootBuilder); + DocumentMapperParser parser = new DocumentMapperParser(index, settings, mapperService, analysisService, similarityLookupService, null); + this.docMapper = b.build(null, parser); } diff --git a/src/test/java/org/elasticsearch/index/mapper/core/BooleanFieldMapperTests.java b/src/test/java/org/elasticsearch/index/mapper/core/BooleanFieldMapperTests.java index 533052995d3..7269605fe64 100644 --- a/src/test/java/org/elasticsearch/index/mapper/core/BooleanFieldMapperTests.java +++ b/src/test/java/org/elasticsearch/index/mapper/core/BooleanFieldMapperTests.java @@ -35,9 +35,7 @@ import org.elasticsearch.index.IndexService; import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.DocumentMapperParser; import org.elasticsearch.index.mapper.FieldMapper; -import org.elasticsearch.index.mapper.FieldMapperListener; import org.elasticsearch.index.mapper.ParsedDocument; -import org.elasticsearch.index.mapper.core.BooleanFieldMapper; import org.elasticsearch.test.ElasticsearchSingleNodeTest; import org.junit.Before; diff --git a/src/test/java/org/elasticsearch/index/mapper/externalvalues/ExternalMapper.java b/src/test/java/org/elasticsearch/index/mapper/externalvalues/ExternalMapper.java index d9793aa228d..17815ebf9d1 100755 --- a/src/test/java/org/elasticsearch/index/mapper/externalvalues/ExternalMapper.java +++ b/src/test/java/org/elasticsearch/index/mapper/externalvalues/ExternalMapper.java @@ -32,12 +32,10 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.fielddata.FieldDataType; import org.elasticsearch.index.mapper.ContentPath; import org.elasticsearch.index.mapper.FieldMapper; -import org.elasticsearch.index.mapper.FieldMapperListener; import org.elasticsearch.index.mapper.Mapper; import org.elasticsearch.index.mapper.MapperParsingException; -import org.elasticsearch.index.mapper.MergeResult; import org.elasticsearch.index.mapper.MergeMappingException; -import org.elasticsearch.index.mapper.ObjectMapperListener; +import org.elasticsearch.index.mapper.MergeResult; import org.elasticsearch.index.mapper.ParseContext; import org.elasticsearch.index.mapper.core.AbstractFieldMapper; import org.elasticsearch.index.mapper.core.BinaryFieldMapper; diff --git a/src/test/java/org/elasticsearch/index/mapper/multifield/MultiFieldTests.java b/src/test/java/org/elasticsearch/index/mapper/multifield/MultiFieldTests.java index 75c58a7bf8f..477f2b60c85 100644 --- a/src/test/java/org/elasticsearch/index/mapper/multifield/MultiFieldTests.java +++ b/src/test/java/org/elasticsearch/index/mapper/multifield/MultiFieldTests.java @@ -147,7 +147,7 @@ public class MultiFieldTests extends ElasticsearchSingleNodeTest { stringField("name").store(true) .addMultiField(stringField("indexed").index(true).tokenized(true)) .addMultiField(stringField("not_indexed").index(false).store(true)) - )).build(mapperParser); + )).build(indexService.mapperService(), mapperParser); builderDocMapper.refreshSource(); String builtMapping = builderDocMapper.mappingSource().string(); diff --git a/src/test/java/org/elasticsearch/index/mapper/simple/SimpleMapperTests.java b/src/test/java/org/elasticsearch/index/mapper/simple/SimpleMapperTests.java index bcc4bab66ac..02ad4ab113b 100644 --- a/src/test/java/org/elasticsearch/index/mapper/simple/SimpleMapperTests.java +++ b/src/test/java/org/elasticsearch/index/mapper/simple/SimpleMapperTests.java @@ -48,7 +48,7 @@ public class SimpleMapperTests extends ElasticsearchSingleNodeTest { DocumentMapper docMapper = doc("test", settings, rootObject("person") .add(object("name").add(stringField("first").store(true).index(false))) - ).build(mapperParser); + ).build(indexService.mapperService(), mapperParser); BytesReference json = new BytesArray(copyToBytesFromClasspath("/org/elasticsearch/index/mapper/simple/test1.json")); Document doc = docMapper.parse("person", "1", json).rootDoc(); @@ -126,7 +126,7 @@ public class SimpleMapperTests extends ElasticsearchSingleNodeTest { DocumentMapper docMapper = doc("test", settings, rootObject("person") .add(object("name").add(stringField("first").store(true).index(false))) - ).build(mapperParser); + ).build(indexService.mapperService(), mapperParser); BytesReference json = new BytesArray("".getBytes(Charsets.UTF_8)); try {