Skip to content

Commit

Permalink
Make mappings immutable.
Browse files Browse the repository at this point in the history 
Today mappings are mutable because of two APIs:
 - Mapper.merge, which expects changes to be performed in-place
 - IncludeInAll, which allows to change whether values should be put in the
   `_all` field in place.

This commit changes both APIs to return a modified copy instead of modifying in
place so that mappings can be immutable. For now, only the type-level object is
immutable, but in the future we can imagine making them immutable at the
index-level so that mapping updates could be completely atomic at the index
level.

Close elastic#9365
  • Loading branch information
@jpountz
jpountz committed Dec 15, 2015
1 parent 5f7b863 commit 50eeafa
Show file tree
Hide file tree
Showing 59 changed files with 437 additions and 614 deletions.
8 changes: 2 additions & 6 deletions core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataMappingService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import org.elasticsearch.index.NodeServicesProvider;
import org.elasticsearch.index.mapper.DocumentMapper;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.MergeResult;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.indices.InvalidTypeNameException;
import org.elasticsearch.percolator.PercolatorService;
Expand Down Expand Up @@ -251,11 +250,8 @@ private ClusterState applyRequest(ClusterState currentState, PutMappingClusterSt
newMapper = indexService.mapperService().parse(request.type(), new CompressedXContent(request.source()), existingMapper == null);
if (existingMapper != null) {
// first, simulate
MergeResult mergeResult = existingMapper.merge(newMapper.mapping(), true, request.updateAllTypes());
// if we have conflicts, throw an exception
if (mergeResult.hasConflicts()) {
throw new IllegalArgumentException("Merge failed with failures {" + Arrays.toString(mergeResult.buildConflicts()) + "}");
}
// this will just throw exceptions in case of problems
existingMapper.merge(newMapper.mapping(), true, request.updateAllTypes());
} else {
// TODO: can we find a better place for this validation?
// The reason this validation is here is that the mapper service doesn't learn about
Expand Down
16 changes: 10 additions & 6 deletions core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -117,7 +118,7 @@ public DocumentMapper build(MapperService mapperService, DocumentMapperParser do

private volatile CompressedXContent mappingSource;

private final Mapping mapping;
private volatile Mapping mapping;

private final DocumentParser documentParser;

Expand Down Expand Up @@ -352,16 +353,19 @@ private void addMappers(Collection<ObjectMapper> objectMappers, Collection<Field
mapperService.addMappers(type, objectMappers, fieldMappers);
}

public MergeResult merge(Mapping mapping, boolean simulate, boolean updateAllTypes) {
public void merge(Mapping mapping, boolean simulate, boolean updateAllTypes) {
try (ReleasableLock lock = mappingWriteLock.acquire()) {
mapperService.checkMappersCompatibility(type, mapping, updateAllTypes);
final MergeResult mergeResult = new MergeResult(simulate, updateAllTypes);
this.mapping.merge(mapping, mergeResult);
// do the merge even if simulate == false so that we get exceptions
Mapping merged = this.mapping.merge(mapping, updateAllTypes);
if (simulate == false) {
addMappers(mergeResult.getNewObjectMappers(), mergeResult.getNewFieldMappers(), updateAllTypes);
this.mapping = merged;
Collection<ObjectMapper> objectMappers = new ArrayList<>();
Collection<FieldMapper> fieldMappers = new ArrayList<>(Arrays.asList(merged.metadataMappers));
MapperUtils.collect(merged.root, objectMappers, fieldMappers);
addMappers(objectMappers, fieldMappers, updateAllTypes);
refreshSource();
}
return mergeResult;
}
}

Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ static ObjectMapper parseObject(ParseContext context, ObjectMapper mapper, boole
if (update == null) {
update = newUpdate;
} else {
MapperUtils.merge(update, newUpdate);
update = update.merge(newUpdate, false);
}
}
}
Expand Down Expand Up @@ -759,7 +759,7 @@ private static void parseCopy(String field, ParseContext context) throws IOExcep
private static <M extends Mapper> M parseAndMergeUpdate(M mapper, ParseContext context) throws IOException {
final Mapper update = parseObjectOrField(context, mapper);
if (update != null) {
MapperUtils.merge(mapper, update);
mapper = (M) mapper.merge(update, false);
}
return mapper;
}
Expand Down
112 changes: 58 additions & 54 deletions core/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
import java.util.Locale;
import java.util.stream.StreamSupport;

public abstract class FieldMapper extends Mapper {
public abstract class FieldMapper extends Mapper implements Cloneable {

public abstract static class Builder<T extends Builder, Y extends FieldMapper> extends Mapper.Builder<T, Y> {

Expand Down Expand Up @@ -84,8 +84,13 @@ public T index(boolean index) {
* if the fieldType has a non-null option we are all good it might have been set through a different
* call.
*/
final IndexOptions options = getDefaultIndexOption();
assert options != IndexOptions.NONE : "default IndexOptions is NONE can't enable indexing";
IndexOptions options = getDefaultIndexOption();
if (options == IndexOptions.NONE) {
// can happen when an existing type on the same index has disabled indexing
// since we inherit the default field type from the first mapper that is
// created on an index
throw new IllegalArgumentException("mapper [" + name + "] has different [index] values from other types of the same index");
}
fieldType.setIndexOptions(options);
}
} else {
Expand Down Expand Up @@ -270,7 +275,7 @@ protected void setupFieldType(BuilderContext context) {

protected MappedFieldTypeReference fieldTypeRef;
protected final MappedFieldType defaultFieldType;
protected final MultiFields multiFields;
protected MultiFields multiFields;
protected CopyTo copyTo;
protected final boolean indexCreatedBefore2x;

Expand Down Expand Up @@ -359,26 +364,41 @@ public Iterator<Mapper> iterator() {
}

@Override
public void merge(Mapper mergeWith, MergeResult mergeResult) {
protected FieldMapper clone() {
try {
return (FieldMapper) super.clone();
} catch (CloneNotSupportedException e) {
throw new AssertionError(e);
}
}

@Override
public FieldMapper merge(Mapper mergeWith, boolean updateAllTypes) {
FieldMapper merged = clone();
merged.doMerge(mergeWith, updateAllTypes);
return merged;
}

/**
* Merge changes coming from {@code mergeWith} in place.
* @param updateAllTypes TODO
*/
protected void doMerge(Mapper mergeWith, boolean updateAllTypes) {
if (!this.getClass().equals(mergeWith.getClass())) {
String mergedType = mergeWith.getClass().getSimpleName();
if (mergeWith instanceof FieldMapper) {
mergedType = ((FieldMapper) mergeWith).contentType();
}
mergeResult.addConflict("mapper [" + fieldType().names().fullName() + "] of different type, current_type [" + contentType() + "], merged_type [" + mergedType + "]");
// different types, return
return;
throw new IllegalArgumentException("mapper [" + fieldType().names().fullName() + "] of different type, current_type [" + contentType() + "], merged_type [" + mergedType + "]");
}
FieldMapper fieldMergeWith = (FieldMapper) mergeWith;
multiFields.merge(mergeWith, mergeResult);
multiFields = multiFields.merge(fieldMergeWith.multiFields);

if (mergeResult.simulate() == false && mergeResult.hasConflicts() == false) {
// apply changeable values
MappedFieldType fieldType = fieldMergeWith.fieldType().clone();
fieldType.freeze();
fieldTypeRef.set(fieldType);
this.copyTo = fieldMergeWith.copyTo;
}
// apply changeable values
MappedFieldType fieldType = fieldMergeWith.fieldType().clone();
fieldType.freeze();
fieldTypeRef.set(fieldType);
this.copyTo = fieldMergeWith.copyTo;
}

@Override
Expand Down Expand Up @@ -565,18 +585,20 @@ public MultiFields build(FieldMapper.Builder mainFieldBuilder, BuilderContext co
}

private final ContentPath.Type pathType;
private volatile ImmutableOpenMap<String, FieldMapper> mappers;
private final ImmutableOpenMap<String, FieldMapper> mappers;

public MultiFields(ContentPath.Type pathType, ImmutableOpenMap<String, FieldMapper> mappers) {
private MultiFields(ContentPath.Type pathType, ImmutableOpenMap<String, FieldMapper> mappers) {
this.pathType = pathType;
this.mappers = mappers;
ImmutableOpenMap.Builder<String, FieldMapper> builder = new ImmutableOpenMap.Builder<>();
// we disable the all in multi-field mappers
for (ObjectCursor<FieldMapper> cursor : mappers.values()) {
for (ObjectObjectCursor<String, FieldMapper> cursor : mappers) {
FieldMapper mapper = cursor.value;
if (mapper instanceof AllFieldMapper.IncludeInAll) {
((AllFieldMapper.IncludeInAll) mapper).unsetIncludeInAll();
mapper = (FieldMapper) ((AllFieldMapper.IncludeInAll) mapper).unsetIncludeInAll();
}
builder.put(cursor.key, mapper);
}
this.mappers = builder.build();
}

public void parse(FieldMapper mainField, ParseContext context) throws IOException {
Expand All @@ -598,47 +620,29 @@ public void parse(FieldMapper mainField, ParseContext context) throws IOExceptio
context.path().pathType(origPathType);
}

// No need for locking, because locking is taken care of in ObjectMapper#merge and DocumentMapper#merge
public void merge(Mapper mergeWith, MergeResult mergeResult) {
FieldMapper mergeWithMultiField = (FieldMapper) mergeWith;

List<FieldMapper> newFieldMappers = null;
ImmutableOpenMap.Builder<String, FieldMapper> newMappersBuilder = null;
public MultiFields merge(MultiFields mergeWith) {
if (pathType != mergeWith.pathType) {
throw new IllegalArgumentException("Can't change path type from [" + pathType + "] to [" + mergeWith.pathType + "]");
}
ImmutableOpenMap.Builder<String, FieldMapper> newMappersBuilder = ImmutableOpenMap.builder(mappers);

for (ObjectCursor<FieldMapper> cursor : mergeWithMultiField.multiFields.mappers.values()) {
for (ObjectCursor<FieldMapper> cursor : mergeWith.mappers.values()) {
FieldMapper mergeWithMapper = cursor.value;
Mapper mergeIntoMapper = mappers.get(mergeWithMapper.simpleName());
FieldMapper mergeIntoMapper = mappers.get(mergeWithMapper.simpleName());
if (mergeIntoMapper == null) {
// no mapping, simply add it if not simulating
if (!mergeResult.simulate()) {
// we disable the all in multi-field mappers
if (mergeWithMapper instanceof AllFieldMapper.IncludeInAll) {
((AllFieldMapper.IncludeInAll) mergeWithMapper).unsetIncludeInAll();
}
if (newMappersBuilder == null) {
newMappersBuilder = ImmutableOpenMap.builder(mappers);
}
newMappersBuilder.put(mergeWithMapper.simpleName(), mergeWithMapper);
if (mergeWithMapper instanceof FieldMapper) {
if (newFieldMappers == null) {
newFieldMappers = new ArrayList<>(2);
}
newFieldMappers.add(mergeWithMapper);
}
// we disable the all in multi-field mappers
if (mergeWithMapper instanceof AllFieldMapper.IncludeInAll) {
mergeWithMapper = (FieldMapper) ((AllFieldMapper.IncludeInAll) mergeWithMapper).unsetIncludeInAll();
}
newMappersBuilder.put(mergeWithMapper.simpleName(), mergeWithMapper);
} else {
mergeIntoMapper.merge(mergeWithMapper, mergeResult);
FieldMapper merged = mergeIntoMapper.merge(mergeWithMapper, false);
newMappersBuilder.put(merged.simpleName(), merged); // override previous definition
}
}

// first add all field mappers
if (newFieldMappers != null) {
mergeResult.addFieldMappers(newFieldMappers);
}
// now publish mappers
if (newMappersBuilder != null) {
mappers = newMappersBuilder.build();
}
ImmutableOpenMap<String, FieldMapper> mappers = newMappersBuilder.build();
return new MultiFields(pathType, mappers);
}

public Iterator<Mapper> iterator() {
Expand Down
4 changes: 3 additions & 1 deletion core/src/main/java/org/elasticsearch/index/mapper/Mapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,5 +174,7 @@ public final String simpleName() {
/** Returns the canonical name which uniquely identifies the mapper against other mappers in a type. */
public abstract String name();

public abstract void merge(Mapper mergeWith, MergeResult mergeResult);
/** Return the merge of {@code mergeWith} into this.
* Both {@code this} and {@code mergeWith} will be left unmodified. */
public abstract Mapper merge(Mapper mergeWith, boolean updateAllTypes);
}
22 changes: 5 additions & 17 deletions core/src/main/java/org/elasticsearch/index/mapper/MapperService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,14 +251,7 @@ private DocumentMapper merge(DocumentMapper mapper, boolean updateAllTypes) {
DocumentMapper oldMapper = mappers.get(mapper.type());

if (oldMapper != null) {
// simulate first
MergeResult result = oldMapper.merge(mapper.mapping(), true, updateAllTypes);
if (result.hasConflicts()) {
throw new IllegalArgumentException("Merge failed with failures {" + Arrays.toString(result.buildConflicts()) + "}");
}
// then apply for real
result = oldMapper.merge(mapper.mapping(), false, updateAllTypes);
assert result.hasConflicts() == false; // we already simulated
oldMapper.merge(mapper.mapping(), false, updateAllTypes);
return oldMapper;
} else {
Tuple<Collection<ObjectMapper>, Collection<FieldMapper>> newMappers = checkMappersCompatibility(
Expand Down Expand Up @@ -305,12 +298,9 @@ protected void checkMappersCompatibility(String type, Collection<ObjectMapper> o
for (ObjectMapper newObjectMapper : objectMappers) {
ObjectMapper existingObjectMapper = fullPathObjectMappers.get(newObjectMapper.fullPath());
if (existingObjectMapper != null) {
MergeResult result = new MergeResult(true, updateAllTypes);
existingObjectMapper.merge(newObjectMapper, result);
if (result.hasConflicts()) {
throw new IllegalArgumentException("Mapper for [" + newObjectMapper.fullPath() + "] conflicts with existing mapping in other types" +
Arrays.toString(result.buildConflicts()));
}
// simulate a merge and ignore the result, we are just interested
// in exceptions here
existingObjectMapper.merge(newObjectMapper, updateAllTypes);
}
}
fieldTypes.checkCompatibility(type, fieldMappers, updateAllTypes);
Expand All @@ -320,9 +310,7 @@ protected Tuple<Collection<ObjectMapper>, Collection<FieldMapper>> checkMappersC
String type, Mapping mapping, boolean updateAllTypes) {
List<ObjectMapper> objectMappers = new ArrayList<>();
List<FieldMapper> fieldMappers = new ArrayList<>();
for (MetadataFieldMapper metadataMapper : mapping.metadataMappers) {
fieldMappers.add(metadataMapper);
}
Collections.addAll(fieldMappers, mapping.metadataMappers);
MapperUtils.collect(mapping.root, objectMappers, fieldMappers);
checkMappersCompatibility(type, objectMappers, fieldMappers, updateAllTypes);
return new Tuple<>(objectMappers, fieldMappers);
Expand Down
46 changes: 0 additions & 46 deletions core/src/main/java/org/elasticsearch/index/mapper/MapperUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,52 +27,6 @@
public enum MapperUtils {
;

private static MergeResult newStrictMergeResult() {
return new MergeResult(false, false) {

@Override
public void addFieldMappers(Collection<FieldMapper> fieldMappers) {
// no-op
}

@Override
public void addObjectMappers(Collection<ObjectMapper> objectMappers) {
// no-op
}

@Override
public Collection<FieldMapper> getNewFieldMappers() {
throw new UnsupportedOperationException("Strict merge result does not support new field mappers");
}

@Override
public Collection<ObjectMapper> getNewObjectMappers() {
throw new UnsupportedOperationException("Strict merge result does not support new object mappers");
}

@Override
public void addConflict(String mergeFailure) {
throw new MapperParsingException("Merging dynamic updates triggered a conflict: " + mergeFailure);
}
};
}

/**
* Merge {@code mergeWith} into {@code mergeTo}. Note: this method only
* merges mappings, not lookup structures. Conflicts are returned as exceptions.
*/
public static void merge(Mapper mergeInto, Mapper mergeWith) {
mergeInto.merge(mergeWith, newStrictMergeResult());
}

/**
* Merge {@code mergeWith} into {@code mergeTo}. Note: this method only
* merges mappings, not lookup structures. Conflicts are returned as exceptions.
*/
public static void merge(Mapping mergeInto, Mapping mergeWith) {
mergeInto.merge(mergeWith, newStrictMergeResult());
}

/** Split mapper and its descendants into object and field mappers. */
public static void collect(Mapper mapper, Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers) {
if (mapper instanceof RootObjectMapper) {
Expand Down
Loading

0 comments on commit 50eeafa

Please sign in to comment.