From 3fd7a36f91c035b4c00f2615f95929d54aab0695 Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Tue, 21 Jul 2015 15:32:31 +0200 Subject: [PATCH] Remove the dependecy on IndexFielddataService from MapperService. This dependency was used in order for mapping updates that change the fielddata format to take effect immediately. And the way it worked was by clearing the cache of fielddata instances that were already loaded. However, we do not need to cache the already loaded (logical) fielddata instances, they are cheap to regenerate. Note that the fielddata _caches_ are still kept around so that we don't keep on rebuilding costly (physical) fielddata values. --- .../fielddata/IndexFieldDataService.java | 117 ++++++------------ .../index/mapper/MapperService.java | 6 +- .../index/engine/InternalEngineTests.java | 3 +- .../fielddata/IndexFieldDataServiceTests.java | 1 - 4 files changed, 42 insertions(+), 85 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/fielddata/IndexFieldDataService.java b/core/src/main/java/org/elasticsearch/index/fielddata/IndexFieldDataService.java index f851420e0367a..287d18c605152 100644 --- a/core/src/main/java/org/elasticsearch/index/fielddata/IndexFieldDataService.java +++ b/core/src/main/java/org/elasticsearch/index/fielddata/IndexFieldDataService.java @@ -26,7 +26,6 @@ import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.util.concurrent.ConcurrentCollections; import org.elasticsearch.common.util.concurrent.KeyedLock; import org.elasticsearch.index.AbstractIndexComponent; import org.elasticsearch.index.Index; @@ -44,7 +43,6 @@ import java.util.Collection; import java.util.List; import java.util.Map; -import java.util.concurrent.ConcurrentMap; import static org.elasticsearch.index.mapper.MappedFieldType.Names; @@ -138,7 +136,6 @@ public class IndexFieldDataService extends AbstractIndexComponent { } private final IndicesFieldDataCache indicesFieldDataCache; - private final ConcurrentMap> loadedFieldData = ConcurrentCollections.newConcurrentMap(); private final KeyedLock.GlobalLockable fieldLoadingLock = new KeyedLock.GlobalLockable<>(); private final Map fieldDataCaches = Maps.newHashMap(); // no need for concurrency support, always used under lock @@ -161,15 +158,6 @@ public void clear() { fieldLoadingLock.globalLock().lock(); try { List exceptions = new ArrayList<>(0); - final Collection> fieldDataValues = loadedFieldData.values(); - for (IndexFieldData fieldData : fieldDataValues) { - try { - fieldData.clear(); - } catch (Throwable t) { - exceptions.add(t); - } - } - fieldDataValues.clear(); final Collection fieldDataCacheValues = fieldDataCaches.values(); for (IndexFieldDataCache cache : fieldDataCacheValues) { try { @@ -189,14 +177,6 @@ public void clearField(final String fieldName) { fieldLoadingLock.acquire(fieldName); try { List exceptions = new ArrayList<>(0); - final IndexFieldData fieldData = loadedFieldData.remove(fieldName); - if (fieldData != null) { - try { - fieldData.clear(); - } catch (Throwable t) { - exceptions.add(t); - } - } final IndexFieldDataCache cache = fieldDataCaches.remove(fieldName); if (cache != null) { try { @@ -211,17 +191,6 @@ public void clearField(final String fieldName) { } } - public void onMappingUpdate() { - // synchronize to make sure to not miss field data instances that are being loaded - fieldLoadingLock.globalLock().lock(); - try { - // important: do not clear fieldDataCaches: the cache may be reused - loadedFieldData.clear(); - } finally { - fieldLoadingLock.globalLock().unlock(); - } - } - @SuppressWarnings("unchecked") public > IFD getForField(MappedFieldType fieldType) { final Names fieldNames = fieldType.names(); @@ -231,57 +200,49 @@ public > IFD getForField(MappedFieldType fieldType } final boolean docValues = fieldType.hasDocValues(); final String key = fieldNames.indexName(); - IndexFieldData fieldData = loadedFieldData.get(key); - if (fieldData == null) { - fieldLoadingLock.acquire(key); - try { - fieldData = loadedFieldData.get(key); - if (fieldData == null) { - IndexFieldData.Builder builder = null; - String format = type.getFormat(indexSettings); - if (format != null && FieldDataType.DOC_VALUES_FORMAT_VALUE.equals(format) && !docValues) { - logger.warn("field [" + fieldNames.fullName() + "] has no doc values, will use default field data format"); - format = null; - } - if (format != null) { - builder = buildersByTypeAndFormat.get(Tuple.tuple(type.getType(), format)); - if (builder == null) { - logger.warn("failed to find format [" + format + "] for field [" + fieldNames.fullName() + "], will use default"); - } - } - if (builder == null && docValues) { - builder = docValuesBuildersByType.get(type.getType()); - } - if (builder == null) { - builder = buildersByType.get(type.getType()); - } - if (builder == null) { - throw new IllegalArgumentException("failed to find field data builder for field " + fieldNames.fullName() + ", and type " + type.getType()); - } - - IndexFieldDataCache cache = fieldDataCaches.get(fieldNames.indexName()); - if (cache == null) { - // we default to node level cache, which in turn defaults to be unbounded - // this means changing the node level settings is simple, just set the bounds there - String cacheType = type.getSettings().get("cache", indexSettings.get(FIELDDATA_CACHE_KEY, FIELDDATA_CACHE_VALUE_NODE)); - if (FIELDDATA_CACHE_VALUE_NODE.equals(cacheType)) { - cache = indicesFieldDataCache.buildIndexFieldDataCache(indexService, index, fieldNames, type); - } else if ("none".equals(cacheType)){ - cache = new IndexFieldDataCache.None(); - } else { - throw new IllegalArgumentException("cache type not supported [" + cacheType + "] for field [" + fieldNames.fullName() + "]"); - } - fieldDataCaches.put(fieldNames.indexName(), cache); - } + fieldLoadingLock.acquire(key); + try { + IndexFieldData.Builder builder = null; + String format = type.getFormat(indexSettings); + if (format != null && FieldDataType.DOC_VALUES_FORMAT_VALUE.equals(format) && !docValues) { + logger.warn("field [" + fieldNames.fullName() + "] has no doc values, will use default field data format"); + format = null; + } + if (format != null) { + builder = buildersByTypeAndFormat.get(Tuple.tuple(type.getType(), format)); + if (builder == null) { + logger.warn("failed to find format [" + format + "] for field [" + fieldNames.fullName() + "], will use default"); + } + } + if (builder == null && docValues) { + builder = docValuesBuildersByType.get(type.getType()); + } + if (builder == null) { + builder = buildersByType.get(type.getType()); + } + if (builder == null) { + throw new IllegalArgumentException("failed to find field data builder for field " + fieldNames.fullName() + ", and type " + type.getType()); + } - fieldData = builder.build(index, indexSettings, fieldType, cache, circuitBreakerService, indexService.mapperService()); - loadedFieldData.put(fieldNames.indexName(), fieldData); + IndexFieldDataCache cache = fieldDataCaches.get(fieldNames.indexName()); + if (cache == null) { + // we default to node level cache, which in turn defaults to be unbounded + // this means changing the node level settings is simple, just set the bounds there + String cacheType = type.getSettings().get("cache", indexSettings.get(FIELDDATA_CACHE_KEY, FIELDDATA_CACHE_VALUE_NODE)); + if (FIELDDATA_CACHE_VALUE_NODE.equals(cacheType)) { + cache = indicesFieldDataCache.buildIndexFieldDataCache(indexService, index, fieldNames, type); + } else if ("none".equals(cacheType)){ + cache = new IndexFieldDataCache.None(); + } else { + throw new IllegalArgumentException("cache type not supported [" + cacheType + "] for field [" + fieldNames.fullName() + "]"); } - } finally { - fieldLoadingLock.release(key); + fieldDataCaches.put(fieldNames.indexName(), cache); } + + return (IFD) builder.build(index, indexSettings, fieldType, cache, circuitBreakerService, indexService.mapperService()); + } finally { + fieldLoadingLock.release(key); } - return (IFD) fieldData; } } 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 cf5d9f4edbd27..8f20ef5f7ab5e 100755 --- a/core/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -53,7 +53,6 @@ import org.elasticsearch.index.AbstractIndexComponent; import org.elasticsearch.index.Index; import org.elasticsearch.index.analysis.AnalysisService; -import org.elasticsearch.index.fielddata.IndexFieldDataService; import org.elasticsearch.index.mapper.Mapper.BuilderContext; import org.elasticsearch.index.mapper.internal.TypeFieldMapper; import org.elasticsearch.index.mapper.object.ObjectMapper; @@ -104,7 +103,6 @@ public Analyzer apply(MappedFieldType fieldType) { }; private final AnalysisService analysisService; - private final IndexFieldDataService fieldDataService; /** * Will create types automatically if they do not exists in the mapping definition yet @@ -139,12 +137,11 @@ public Analyzer apply(MappedFieldType fieldType) { private volatile ImmutableSet parentTypes = ImmutableSet.of(); @Inject - public MapperService(Index index, @IndexSettings Settings indexSettings, AnalysisService analysisService, IndexFieldDataService fieldDataService, + public MapperService(Index index, @IndexSettings Settings indexSettings, AnalysisService analysisService, SimilarityLookupService similarityLookupService, ScriptService scriptService) { super(index, indexSettings); this.analysisService = analysisService; - this.fieldDataService = fieldDataService; this.fieldTypes = new FieldTypeLookup(); this.documentParser = new DocumentMapperParser(indexSettings, this, analysisService, similarityLookupService, scriptService); this.indexAnalyzer = new MapperAnalyzerWrapper(analysisService.defaultIndexAnalyzer(), INDEX_ANALYZER_EXTRACTOR); @@ -291,7 +288,6 @@ private DocumentMapper merge(DocumentMapper mapper, boolean updateAllTypes) { logger.debug("merging mapping for type [{}] resulted in conflicts: [{}]", mapper.type(), Arrays.toString(result.buildConflicts())); } } - fieldDataService.onMappingUpdate(); return oldMapper; } else { List newObjectMappers = new ArrayList<>(); diff --git a/core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java index 6856eb9d0ea77..45a730a50f055 100644 --- a/core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java +++ b/core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java @@ -101,6 +101,7 @@ import static org.elasticsearch.index.engine.Engine.Operation.Origin.PRIMARY; import static org.elasticsearch.index.engine.Engine.Operation.Origin.REPLICA; import static org.hamcrest.Matchers.*; + public class InternalEngineTests extends ElasticsearchTestCase { private static final Pattern PARSE_LEGACY_ID_PATTERN = Pattern.compile("^" + Translog.TRANSLOG_FILE_PREFIX + "(\\d+)((\\.recovering))?$"); @@ -1923,7 +1924,7 @@ public TranslogHandler(String indexName) { 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); + MapperService mapperService = new MapperService(index, settings, analysisService, similarityLookupService, null); DocumentMapper.Builder b = new DocumentMapper.Builder(settings, rootBuilder, mapperService); DocumentMapperParser parser = new DocumentMapperParser(settings, mapperService, analysisService, similarityLookupService, null); this.docMapper = b.build(mapperService, parser); diff --git a/core/src/test/java/org/elasticsearch/index/fielddata/IndexFieldDataServiceTests.java b/core/src/test/java/org/elasticsearch/index/fielddata/IndexFieldDataServiceTests.java index 3af20da91ede5..2823566fddda0 100644 --- a/core/src/test/java/org/elasticsearch/index/fielddata/IndexFieldDataServiceTests.java +++ b/core/src/test/java/org/elasticsearch/index/fielddata/IndexFieldDataServiceTests.java @@ -150,7 +150,6 @@ public void testChangeFieldDataFormat() throws Exception { writer.addDocument(doc); final IndexReader reader2 = DirectoryReader.open(writer, true); final MappedFieldType mapper2 = MapperBuilders.stringField("s").tokenized(false).docValues(true).fieldDataSettings(Settings.builder().put(FieldDataType.FORMAT_KEY, "doc_values").build()).build(ctx).fieldType(); - ifdService.onMappingUpdate(); ifd = ifdService.getForField(mapper2); assertThat(ifd, instanceOf(SortedSetDVOrdinalsIndexFieldData.class)); reader1.close();