Skip to content

Commit

Permalink
Make FieldTypeLookup immutable (#58162) (#58411)
Browse files Browse the repository at this point in the history 
FieldTypeLookup maps field names to their MappedFieldTypes. In the past, due to
the presence of multiple mapping types within a single index, this had to be updated
in-place because a mapping update might only affect one type. However, now that
we only have a single type per index, we can completely rebuild the FieldTypeLookup
on each update, removing lots of concurrency worries.
  • Loading branch information
@romseygeek
romseygeek committed Jun 23, 2020
1 parent f97b371 commit 519d127
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 185 deletions.
27 changes: 5 additions & 22 deletions server/src/main/java/org/elasticsearch/index/mapper/DynamicKeyFieldTypeLookup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@

package org.elasticsearch.index.mapper;

import org.elasticsearch.common.collect.CopyOnWriteHashMap;

import java.util.Collections;
import java.util.Iterator;
import java.util.Map;

Expand All @@ -36,7 +33,7 @@
* Flattened object fields live in the 'mapper-flattened' module.
*/
class DynamicKeyFieldTypeLookup {
private final CopyOnWriteHashMap<String, DynamicKeyFieldMapper> mappers;
private final Map<String, DynamicKeyFieldMapper> mappers;
private final Map<String, String> aliasToConcreteName;

/**
Expand All @@ -45,25 +42,11 @@ class DynamicKeyFieldTypeLookup {
*/
private final int maxKeyDepth;

DynamicKeyFieldTypeLookup() {
this.mappers = new CopyOnWriteHashMap<>();
this.aliasToConcreteName = Collections.emptyMap();
this.maxKeyDepth = 0;
}

private DynamicKeyFieldTypeLookup(CopyOnWriteHashMap<String, DynamicKeyFieldMapper> mappers,
Map<String, String> aliasToConcreteName,
int maxKeyDepth) {
this.mappers = mappers;
DynamicKeyFieldTypeLookup(Map<String, DynamicKeyFieldMapper> newMappers,
Map<String, String> aliasToConcreteName) {
this.mappers = newMappers;
this.aliasToConcreteName = aliasToConcreteName;
this.maxKeyDepth = maxKeyDepth;
}

DynamicKeyFieldTypeLookup copyAndAddAll(Map<String, DynamicKeyFieldMapper> newMappers,
Map<String, String> aliasToConcreteName) {
CopyOnWriteHashMap<String, DynamicKeyFieldMapper> combinedMappers = this.mappers.copyAndPutAll(newMappers);
int maxKeyDepth = getMaxKeyDepth(combinedMappers, aliasToConcreteName);
return new DynamicKeyFieldTypeLookup(combinedMappers, aliasToConcreteName, maxKeyDepth);
this.maxKeyDepth = getMaxKeyDepth(mappers, aliasToConcreteName);
}

/**
Expand Down
49 changes: 9 additions & 40 deletions server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,69 +19,39 @@

package org.elasticsearch.index.mapper;

import org.elasticsearch.common.collect.CopyOnWriteHashMap;
import org.elasticsearch.common.collect.Iterators;
import org.elasticsearch.common.regex.Regex;

import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

/**
* An immutable container for looking up {@link MappedFieldType}s by their name.
*/
class FieldTypeLookup implements Iterable<MappedFieldType> {

final CopyOnWriteHashMap<String, MappedFieldType> fullNameToFieldType;
private final CopyOnWriteHashMap<String, String> aliasToConcreteName;
private final Map<String, MappedFieldType> fullNameToFieldType = new HashMap<>();
private final Map<String, String> aliasToConcreteName = new HashMap<>();
private final DynamicKeyFieldTypeLookup dynamicKeyLookup;


FieldTypeLookup() {
fullNameToFieldType = new CopyOnWriteHashMap<>();
aliasToConcreteName = new CopyOnWriteHashMap<>();
dynamicKeyLookup = new DynamicKeyFieldTypeLookup();
this(Collections.emptyList(), Collections.emptyList());
}

private FieldTypeLookup(CopyOnWriteHashMap<String, MappedFieldType> fullNameToFieldType,
CopyOnWriteHashMap<String, String> aliasToConcreteName,
DynamicKeyFieldTypeLookup dynamicKeyLookup) {
this.fullNameToFieldType = fullNameToFieldType;
this.aliasToConcreteName = aliasToConcreteName;
this.dynamicKeyLookup = dynamicKeyLookup;
}
FieldTypeLookup(Collection<FieldMapper> fieldMappers,
Collection<FieldAliasMapper> fieldAliasMappers) {

/**
* Return a new instance that contains the union of this instance and the field types
* from the provided mappers. If a field already exists, its field type will be updated
* to use the new type from the given field mapper. Similarly if an alias already
* exists, it will be updated to reference the field type from the new mapper.
*/
public FieldTypeLookup copyAndAddAll(String type,
Collection<FieldMapper> fieldMappers,
Collection<FieldAliasMapper> fieldAliasMappers) {
Objects.requireNonNull(type, "type must not be null");
if (MapperService.DEFAULT_MAPPING.equals(type)) {
throw new IllegalArgumentException("Default mappings should not be added to the lookup");
}

CopyOnWriteHashMap<String, MappedFieldType> fullName = this.fullNameToFieldType;
CopyOnWriteHashMap<String, String> aliases = this.aliasToConcreteName;
Map<String, DynamicKeyFieldMapper> dynamicKeyMappers = new HashMap<>();

for (FieldMapper fieldMapper : fieldMappers) {
String fieldName = fieldMapper.name();
MappedFieldType fieldType = fieldMapper.fieldType();
MappedFieldType fullNameFieldType = fullName.get(fieldType.name());

if (!Objects.equals(fieldType, fullNameFieldType)) {
fullName = fullName.copyAndPut(fieldType.name(), fieldType);
}

fullNameToFieldType.put(fieldType.name(), fieldType);
if (fieldMapper instanceof DynamicKeyFieldMapper) {
dynamicKeyMappers.put(fieldName, (DynamicKeyFieldMapper) fieldMapper);
}
Expand All @@ -90,11 +60,10 @@ public FieldTypeLookup copyAndAddAll(String type,
for (FieldAliasMapper fieldAliasMapper : fieldAliasMappers) {
String aliasName = fieldAliasMapper.name();
String path = fieldAliasMapper.path();
aliases = aliases.copyAndPut(aliasName, path);
aliasToConcreteName.put(aliasName, path);
}

DynamicKeyFieldTypeLookup newDynamicKeyLookup = this.dynamicKeyLookup.copyAndAddAll(dynamicKeyMappers, aliases);
return new FieldTypeLookup(fullName, aliases, newDynamicKeyLookup);
this.dynamicKeyLookup = new DynamicKeyFieldTypeLookup(dynamicKeyMappers, aliasToConcreteName);
}

/**
Expand Down
4 changes: 1 addition & 3 deletions server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,10 @@ class MapperMergeValidator {
* @param objectMappers The newly added object mappers.
* @param fieldMappers The newly added field mappers.
* @param fieldAliasMappers The newly added field alias mappers.
* @param fieldTypes Any existing field and field alias mappers, collected into a lookup structure.
*/
public static void validateNewMappers(Collection<ObjectMapper> objectMappers,
Collection<FieldMapper> fieldMappers,
Collection<FieldAliasMapper> fieldAliasMappers,
FieldTypeLookup fieldTypes) {
Collection<FieldAliasMapper> fieldAliasMappers) {
Set<String> objectFullNames = new HashSet<>();
for (ObjectMapper objectMapper : objectMappers) {
String fullPath = objectMapper.fullPath();
Expand Down
12 changes: 6 additions & 6 deletions server/src/main/java/org/elasticsearch/index/mapper/MapperService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,6 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
MergeReason reason) {
boolean hasNested = this.hasNested;
Map<String, ObjectMapper> fullPathObjectMappers = this.fullPathObjectMappers;
FieldTypeLookup fieldTypes = this.fieldTypes;

Map<String, DocumentMapper> results = new LinkedHashMap<>(2);

Expand All @@ -474,6 +473,7 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
}

DocumentMapper newMapper = null;
FieldTypeLookup newFieldTypes = null;
if (mapper != null) {
// check naming
validateTypeName(mapper.type());
Expand All @@ -494,11 +494,11 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
Collections.addAll(fieldMappers, metadataMappers);
MapperUtils.collect(newMapper.mapping().root(), objectMappers, fieldMappers, fieldAliasMappers);

MapperMergeValidator.validateNewMappers(objectMappers, fieldMappers, fieldAliasMappers, fieldTypes);
MapperMergeValidator.validateNewMappers(objectMappers, fieldMappers, fieldAliasMappers);
checkPartitionedIndexConstraints(newMapper);

// update lookup data-structures
fieldTypes = fieldTypes.copyAndAddAll(newMapper.type(), fieldMappers, fieldAliasMappers);
newFieldTypes = new FieldTypeLookup(fieldMappers, fieldAliasMappers);

for (ObjectMapper objectMapper : objectMappers) {
if (fullPathObjectMappers == this.fullPathObjectMappers) {
Expand All @@ -513,9 +513,9 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
}

MapperMergeValidator.validateFieldReferences(fieldMappers, fieldAliasMappers,
fullPathObjectMappers, fieldTypes);
fullPathObjectMappers, newFieldTypes);

ContextMapping.validateContextPaths(indexSettings.getIndexVersionCreated(), fieldMappers, fieldTypes::get);
ContextMapping.validateContextPaths(indexSettings.getIndexVersionCreated(), fieldMappers, newFieldTypes::get);

if (reason == MergeReason.MAPPING_UPDATE || reason == MergeReason.MAPPING_UPDATE_PREFLIGHT) {
// this check will only be performed on the master node when there is
Expand Down Expand Up @@ -563,8 +563,8 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
}
if (newMapper != null) {
this.mapper = newMapper;
this.fieldTypes = newFieldTypes;
}
this.fieldTypes = fieldTypes;
this.hasNested = hasNested;
this.fullPathObjectMappers = fullPathObjectMappers;

Expand Down
102 changes: 7 additions & 95 deletions server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@

import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;

import static java.util.Collections.emptyList;

Expand All @@ -41,111 +41,32 @@ public void testEmpty() {
assertFalse(itr.hasNext());
}

public void testDefaultMapping() {
FieldTypeLookup lookup = new FieldTypeLookup();
try {
lookup.copyAndAddAll(MapperService.DEFAULT_MAPPING, emptyList(), emptyList());
fail();
} catch (IllegalArgumentException expected) {
assertEquals("Default mappings should not be added to the lookup", expected.getMessage());
}
}

public void testAddNewField() {
FieldTypeLookup lookup = new FieldTypeLookup();
MockFieldMapper f = new MockFieldMapper("foo");
FieldTypeLookup lookup2 = lookup.copyAndAddAll("type", newList(f), emptyList());
assertNull(lookup.get("foo"));
FieldTypeLookup lookup = new FieldTypeLookup(Collections.singletonList(f), emptyList());
assertNull(lookup.get("bar"));
assertEquals(f.fieldType(), lookup2.get("foo"));
assertNull(lookup.get("bar"));
assertEquals(1, size(lookup2.iterator()));
}

public void testAddExistingField() {
MockFieldMapper f = new MockFieldMapper("foo");
MockFieldMapper f2 = new MockFieldMapper("foo");
FieldTypeLookup lookup = new FieldTypeLookup();
lookup = lookup.copyAndAddAll("type1", newList(f), emptyList());
FieldTypeLookup lookup2 = lookup.copyAndAddAll("type2", newList(f2), emptyList());

assertEquals(1, size(lookup2.iterator()));
assertSame(f.fieldType(), lookup2.get("foo"));
assertEquals(f2.fieldType(), lookup2.get("foo"));
assertEquals(f.fieldType(), lookup.get("foo"));
assertEquals(1, size(lookup.iterator()));
}

public void testAddFieldAlias() {
MockFieldMapper field = new MockFieldMapper("foo");
FieldAliasMapper alias = new FieldAliasMapper("alias", "alias", "foo");

FieldTypeLookup lookup = new FieldTypeLookup();
lookup = lookup.copyAndAddAll("type", newList(field), newList(alias));
FieldTypeLookup lookup = new FieldTypeLookup(Collections.singletonList(field), Collections.singletonList(alias));

MappedFieldType aliasType = lookup.get("alias");
assertEquals(field.fieldType(), aliasType);
}

public void testUpdateFieldAlias() {
// Add an alias 'alias' to the concrete field 'foo'.
MockFieldMapper.FakeFieldType fieldType1 = new MockFieldMapper.FakeFieldType("foo");
MockFieldMapper field1 = new MockFieldMapper(fieldType1);
FieldAliasMapper alias1 = new FieldAliasMapper("alias", "alias", "foo");

FieldTypeLookup lookup = new FieldTypeLookup();
lookup = lookup.copyAndAddAll("type", newList(field1), newList(alias1));

// Check that the alias refers to 'foo'.
MappedFieldType aliasType1 = lookup.get("alias");
assertEquals(fieldType1, aliasType1);

// Update the alias to refer to a new concrete field 'bar'.
MockFieldMapper.FakeFieldType fieldType2 = new MockFieldMapper.FakeFieldType("bar");
MockFieldMapper field2 = new MockFieldMapper(fieldType2);

FieldAliasMapper alias2 = new FieldAliasMapper("alias", "alias", "bar");
lookup = lookup.copyAndAddAll("type", newList(field2), newList(alias2));

// Check that the alias now refers to 'bar'.
MappedFieldType aliasType2 = lookup.get("alias");
assertEquals(fieldType2, aliasType2);
}

public void testUpdateConcreteFieldWithAlias() {
// Add an alias 'alias' to the concrete field 'foo'.
FieldAliasMapper alias1 = new FieldAliasMapper("alias", "alias", "foo");
MockFieldMapper.FakeFieldType fieldType1 = new MockFieldMapper.FakeFieldType("foo");
fieldType1.setBoost(1.0f);
MockFieldMapper field1 = new MockFieldMapper(fieldType1);

FieldTypeLookup lookup = new FieldTypeLookup();
lookup = lookup.copyAndAddAll("type", newList(field1), newList(alias1));

// Check that the alias maps to this field type.
MappedFieldType aliasType1 = lookup.get("alias");
assertEquals(fieldType1, aliasType1);

// Update the boost for field 'foo'.
MockFieldMapper.FakeFieldType fieldType2 = new MockFieldMapper.FakeFieldType("foo");
fieldType2.setBoost(2.0f);
MockFieldMapper field2 = new MockFieldMapper(fieldType2);
lookup = lookup.copyAndAddAll("type", newList(field2), emptyList());

// Check that the alias maps to the new field type.
MappedFieldType aliasType2 = lookup.get("alias");
assertEquals(fieldType2, aliasType2);
}

public void testSimpleMatchToFullName() {
MockFieldMapper field1 = new MockFieldMapper("foo");
MockFieldMapper field2 = new MockFieldMapper("bar");

FieldAliasMapper alias1 = new FieldAliasMapper("food", "food", "foo");
FieldAliasMapper alias2 = new FieldAliasMapper("barometer", "barometer", "bar");

FieldTypeLookup lookup = new FieldTypeLookup();
lookup = lookup.copyAndAddAll("type",
newList(field1, field2),
newList(alias1, alias2));
FieldTypeLookup lookup = new FieldTypeLookup(Arrays.asList(field1, field2), Arrays.asList(alias1, alias2));

Collection<String> names = lookup.simpleMatchToFullName("b*");

Expand All @@ -158,8 +79,7 @@ public void testSimpleMatchToFullName() {

public void testIteratorImmutable() {
MockFieldMapper f1 = new MockFieldMapper("foo");
FieldTypeLookup lookup = new FieldTypeLookup();
lookup = lookup.copyAndAddAll("type", newList(f1), emptyList());
FieldTypeLookup lookup = new FieldTypeLookup(Collections.singletonList(f1), emptyList());

try {
Iterator<MappedFieldType> itr = lookup.iterator();
Expand All @@ -172,14 +92,6 @@ public void testIteratorImmutable() {
}
}

private static List<FieldMapper> newList(FieldMapper... mapper) {
return Arrays.asList(mapper);
}

private static List<FieldAliasMapper> newList(FieldAliasMapper... mapper) {
return Arrays.asList(mapper);
}

private int size(Iterator<MappedFieldType> iterator) {
if (iterator == null) {
throw new NullPointerException("iterator");
Expand Down

0 comments on commit 519d127

Please sign in to comment.