Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce mapping version to index metadata #33147

Merged
merged 13 commits into from
Aug 27, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ public String toString() {
final String TAB = " ";
for (IndexMetaData indexMetaData : metaData) {
sb.append(TAB).append(indexMetaData.getIndex());
sb.append(": v[").append(indexMetaData.getVersion()).append("]\n");
sb.append(": v[").append(indexMetaData.getVersion()).append("], mv[").append(indexMetaData.getMappingVersion()).append("]\n");
for (int shard = 0; shard < indexMetaData.getNumberOfShards(); shard++) {
sb.append(TAB).append(TAB).append(shard).append(": ");
sb.append("p_term [").append(indexMetaData.primaryTerm(shard)).append("], ");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.carrotsearch.hppc.cursors.ObjectCursor;
import com.carrotsearch.hppc.cursors.ObjectObjectCursor;

import org.elasticsearch.Assertions;
import org.elasticsearch.Version;
import org.elasticsearch.action.admin.indices.rollover.RolloverInfo;
import org.elasticsearch.action.support.ActiveShardCount;
Expand Down Expand Up @@ -291,6 +292,7 @@ public Iterator<Setting<Integer>> settings() {

public static final String KEY_IN_SYNC_ALLOCATIONS = "in_sync_allocations";
static final String KEY_VERSION = "version";
static final String KEY_MAPPING_VERSION = "mapping_version";
static final String KEY_ROUTING_NUM_SHARDS = "routing_num_shards";
static final String KEY_SETTINGS = "settings";
static final String KEY_STATE = "state";
Expand All @@ -309,6 +311,9 @@ public Iterator<Setting<Integer>> settings() {

private final Index index;
private final long version;

private final long mappingVersion;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether MappingMetaData would be a better place to maintain the mapping version. The reason it feels like a better place to me is that this version is about an instance of MappingMetaData.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this but it ends up that we construct mapping metadata in quite a few places where having the mapping version is not natural. We do not hold on to the mapping metadata in the mapping service, so in some of these places it means we would have to hold on to the mapping version in the mapper service. But then we would have multiple places in the system carrying the mapping version. Because of this I considered the index metadata to be better. Additionally, like that we have the builder for the index metadata. Let me know if you feel strongly about this, or see a good way to have it on the mapping metadata.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly about this. I was just wondering about this. Your explanation is a good reason why to add the version to IndexMetadata instead of MappingMetaData.


private final long[] primaryTerms;

private final State state;
Expand Down Expand Up @@ -336,7 +341,7 @@ public Iterator<Setting<Integer>> settings() {
private final ActiveShardCount waitForActiveShards;
private final ImmutableOpenMap<String, RolloverInfo> rolloverInfos;

private IndexMetaData(Index index, long version, long[] primaryTerms, State state, int numberOfShards, int numberOfReplicas, Settings settings,
private IndexMetaData(Index index, long version, long mappingVersion, long[] primaryTerms, State state, int numberOfShards, int numberOfReplicas, Settings settings,
ImmutableOpenMap<String, MappingMetaData> mappings, ImmutableOpenMap<String, AliasMetaData> aliases,
ImmutableOpenMap<String, Custom> customs, ImmutableOpenIntMap<Set<String>> inSyncAllocationIds,
DiscoveryNodeFilters requireFilters, DiscoveryNodeFilters initialRecoveryFilters, DiscoveryNodeFilters includeFilters, DiscoveryNodeFilters excludeFilters,
Expand All @@ -345,6 +350,8 @@ private IndexMetaData(Index index, long version, long[] primaryTerms, State stat

this.index = index;
this.version = version;
assert mappingVersion >= 0 : mappingVersion;
this.mappingVersion = mappingVersion;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we assert that mappingVersion is positive here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed ad6a048.

this.primaryTerms = primaryTerms;
assert primaryTerms.length == numberOfShards;
this.state = state;
Expand Down Expand Up @@ -394,6 +401,9 @@ public long getVersion() {
return this.version;
}

public long getMappingVersion() {
return mappingVersion;
}

/**
* The term of the current selected primary. This is a non-negative number incremented when
Expand Down Expand Up @@ -644,6 +654,7 @@ private static class IndexMetaDataDiff implements Diff<IndexMetaData> {
private final String index;
private final int routingNumShards;
private final long version;
private final long mappingVersion;
private final long[] primaryTerms;
private final State state;
private final Settings settings;
Expand All @@ -656,6 +667,7 @@ private static class IndexMetaDataDiff implements Diff<IndexMetaData> {
IndexMetaDataDiff(IndexMetaData before, IndexMetaData after) {
index = after.index.getName();
version = after.version;
mappingVersion = after.mappingVersion;
routingNumShards = after.routingNumShards;
state = after.state;
settings = after.settings;
Expand All @@ -672,6 +684,11 @@ private static class IndexMetaDataDiff implements Diff<IndexMetaData> {
index = in.readString();
routingNumShards = in.readInt();
version = in.readLong();
if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume when this is backported the version will be changed to 6.5.0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we set the version to 6.5.0 now then the BWC tests will fail.

mappingVersion = in.readVLong();
} else {
mappingVersion = 1;
}
state = State.fromId(in.readByte());
settings = Settings.readSettingsFromStream(in);
primaryTerms = in.readVLongArray();
Expand Down Expand Up @@ -707,6 +724,9 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeString(index);
out.writeInt(routingNumShards);
out.writeLong(version);
if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
out.writeVLong(mappingVersion);
}
out.writeByte(state.id);
Settings.writeSettingsToStream(settings, out);
out.writeVLongArray(primaryTerms);
Expand All @@ -723,6 +743,7 @@ public void writeTo(StreamOutput out) throws IOException {
public IndexMetaData apply(IndexMetaData part) {
Builder builder = builder(index);
builder.version(version);
builder.mappingVersion(mappingVersion);
builder.setRoutingNumShards(routingNumShards);
builder.state(state);
builder.settings(settings);
Expand All @@ -739,6 +760,11 @@ public IndexMetaData apply(IndexMetaData part) {
public static IndexMetaData readFrom(StreamInput in) throws IOException {
Builder builder = new Builder(in.readString());
builder.version(in.readLong());
if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
builder.mappingVersion(in.readVLong());
} else {
builder.mappingVersion(1);
}
builder.setRoutingNumShards(in.readInt());
builder.state(State.fromId(in.readByte()));
builder.settings(readSettingsFromStream(in));
Expand Down Expand Up @@ -778,6 +804,9 @@ public static IndexMetaData readFrom(StreamInput in) throws IOException {
public void writeTo(StreamOutput out) throws IOException {
out.writeString(index.getName()); // uuid will come as part of settings
out.writeLong(version);
if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
out.writeVLong(mappingVersion);
}
out.writeInt(routingNumShards);
out.writeByte(state.id());
writeSettingsToStream(settings, out);
Expand Down Expand Up @@ -821,6 +850,7 @@ public static class Builder {
private String index;
private State state = State.OPEN;
private long version = 1;
private long mappingVersion = 1;
private long[] primaryTerms = null;
private Settings settings = Settings.Builder.EMPTY_SETTINGS;
private final ImmutableOpenMap.Builder<String, MappingMetaData> mappings;
Expand All @@ -843,6 +873,7 @@ public Builder(IndexMetaData indexMetaData) {
this.index = indexMetaData.getIndex().getName();
this.state = indexMetaData.state;
this.version = indexMetaData.version;
this.mappingVersion = indexMetaData.mappingVersion;
this.settings = indexMetaData.getSettings();
this.primaryTerms = indexMetaData.primaryTerms.clone();
this.mappings = ImmutableOpenMap.builder(indexMetaData.mappings);
Expand Down Expand Up @@ -1009,6 +1040,15 @@ public Builder version(long version) {
return this;
}

public long mappingVersion() {
return mappingVersion;
}

public Builder mappingVersion(final long mappingVersion) {
this.mappingVersion = mappingVersion;
return this;
}

/**
* returns the primary term for the given shard.
* See {@link IndexMetaData#primaryTerm(int)} for more information.
Expand Down Expand Up @@ -1136,7 +1176,7 @@ public IndexMetaData build() {

final String uuid = settings.get(SETTING_INDEX_UUID, INDEX_UUID_NA_VALUE);

return new IndexMetaData(new Index(index, uuid), version, primaryTerms, state, numberOfShards, numberOfReplicas, tmpSettings, mappings.build(),
return new IndexMetaData(new Index(index, uuid), version, mappingVersion, primaryTerms, state, numberOfShards, numberOfReplicas, tmpSettings, mappings.build(),
tmpAliases.build(), customs.build(), filledInSyncAllocationIds.build(), requireFilters, initialRecoveryFilters, includeFilters, excludeFilters,
indexCreatedVersion, indexUpgradedVersion, getRoutingNumShards(), routingPartitionSize, waitForActiveShards, rolloverInfos.build());
}
Expand All @@ -1145,6 +1185,7 @@ public static void toXContent(IndexMetaData indexMetaData, XContentBuilder build
builder.startObject(indexMetaData.getIndex().getName());

builder.field(KEY_VERSION, indexMetaData.getVersion());
builder.field(KEY_MAPPING_VERSION, indexMetaData.getMappingVersion());
builder.field(KEY_ROUTING_NUM_SHARDS, indexMetaData.getRoutingNumShards());
builder.field(KEY_STATE, indexMetaData.getState().toString().toLowerCase(Locale.ENGLISH));

Expand Down Expand Up @@ -1218,6 +1259,7 @@ public static IndexMetaData fromXContent(XContentParser parser) throws IOExcepti
if (token != XContentParser.Token.START_OBJECT) {
throw new IllegalArgumentException("expected object but got a " + token);
}
boolean mappingVersion = false;
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
Expand Down Expand Up @@ -1316,6 +1358,9 @@ public static IndexMetaData fromXContent(XContentParser parser) throws IOExcepti
builder.state(State.fromString(parser.text()));
} else if (KEY_VERSION.equals(currentFieldName)) {
builder.version(parser.longValue());
} else if (KEY_MAPPING_VERSION.equals(currentFieldName)) {
mappingVersion = true;
builder.mappingVersion(parser.longValue());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we somehow make sure that if the index is 7.0 that the version is present? I really want to make sure we are not missing it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed e1c6fe6.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what you think of that one @s1monw.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

} else if (KEY_ROUTING_NUM_SHARDS.equals(currentFieldName)) {
builder.setRoutingNumShards(parser.intValue());
} else {
Expand All @@ -1325,6 +1370,9 @@ public static IndexMetaData fromXContent(XContentParser parser) throws IOExcepti
throw new IllegalArgumentException("Unexpected token " + token);
}
}
if (Assertions.ENABLED && Version.indexCreated(builder.settings).onOrAfter(Version.V_7_0_0_alpha1)) {
assert mappingVersion : "mapping version should be present for indices created on or after 7.0.0";
}
return builder.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,14 @@ private ClusterState applyRequest(ClusterState currentState, PutMappingClusterSt
indexMetaDataBuilder.putMapping(new MappingMetaData(mapper.mappingSource()));
}
}
if (updated) {
indexMetaDataBuilder.mappingVersion(1 + indexMetaDataBuilder.mappingVersion());
}
/*
* This implicitly increments the index metadata version and builds the index metadata. This means that we need to have
* already incremented the mapping version if necessary. Therefore, the mapping version increment must remain before this
* statement.
*/
builder.put(indexMetaDataBuilder);
}
if (updated) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -522,8 +522,8 @@ List<SearchOperationListener> getSearchOperationListener() { // pkg private for
}

@Override
public boolean updateMapping(IndexMetaData indexMetaData) throws IOException {
return mapperService().updateMapping(indexMetaData);
public boolean updateMapping(final IndexMetaData currentIndexMetaData, final IndexMetaData newIndexMetaData) throws IOException {
return mapperService().updateMapping(currentIndexMetaData, newIndexMetaData);
}

private class StoreCloseListener implements Store.OnClose {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.lucene.analysis.Analyzer;
import org.apache.lucene.analysis.DelegatingAnalyzerWrapper;
import org.apache.lucene.index.Term;
import org.elasticsearch.Assertions;
import org.elasticsearch.ElasticsearchGenerationException;
import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexMetaData;
Expand Down Expand Up @@ -192,8 +193,8 @@ public static Map<String, Object> parseMapping(NamedXContentRegistry xContentReg
/**
* Update mapping by only merging the metadata that is different between received and stored entries
*/
public boolean updateMapping(IndexMetaData indexMetaData) throws IOException {
assert indexMetaData.getIndex().equals(index()) : "index mismatch: expected " + index() + " but was " + indexMetaData.getIndex();
public boolean updateMapping(final IndexMetaData currentIndexMetaData, final IndexMetaData newIndexMetaData) throws IOException {
assert newIndexMetaData.getIndex().equals(index()) : "index mismatch: expected " + index() + " but was " + newIndexMetaData.getIndex();
// go over and add the relevant mappings (or update them)
Set<String> existingMappers = new HashSet<>();
if (mapper != null) {
Expand All @@ -205,17 +206,19 @@ public boolean updateMapping(IndexMetaData indexMetaData) throws IOException {
final Map<String, DocumentMapper> updatedEntries;
try {
// only update entries if needed
updatedEntries = internalMerge(indexMetaData, MergeReason.MAPPING_RECOVERY, true);
updatedEntries = internalMerge(newIndexMetaData, MergeReason.MAPPING_RECOVERY, true);
} catch (Exception e) {
logger.warn(() -> new ParameterizedMessage("[{}] failed to apply mappings", index()), e);
throw e;
}

boolean requireRefresh = false;

assertMappingVersion(currentIndexMetaData, newIndexMetaData, updatedEntries);

for (DocumentMapper documentMapper : updatedEntries.values()) {
String mappingType = documentMapper.type();
CompressedXContent incomingMappingSource = indexMetaData.mapping(mappingType).source();
CompressedXContent incomingMappingSource = newIndexMetaData.mapping(mappingType).source();

String op = existingMappers.contains(mappingType) ? "updated" : "added";
if (logger.isDebugEnabled() && incomingMappingSource.compressed().length < 512) {
Expand All @@ -240,6 +243,43 @@ public boolean updateMapping(IndexMetaData indexMetaData) throws IOException {
return requireRefresh;
}

private void assertMappingVersion(
final IndexMetaData currentIndexMetaData,
final IndexMetaData newIndexMetaData,
final Map<String, DocumentMapper> updatedEntries) {
if (Assertions.ENABLED && currentIndexMetaData != null) {
if (currentIndexMetaData.getMappingVersion() == newIndexMetaData.getMappingVersion()) {
// if the mapping version is unchanged, then there should not be any updates and all mappings should be the same
assert updatedEntries.isEmpty() : updatedEntries;
for (final ObjectCursor<MappingMetaData> mapping : newIndexMetaData.getMappings().values()) {
final CompressedXContent currentSource = currentIndexMetaData.mapping(mapping.value.type()).source();
final CompressedXContent newSource = mapping.value.source();
assert currentSource.equals(newSource) :
"expected current mapping [" + currentSource + "] for type [" + mapping.value.type() + "] "
+ "to be the same as new mapping [" + newSource + "]";
}
} else {
// if the mapping version is changed, it should increase, there should be updates, and the mapping should be different
final long currentMappingVersion = currentIndexMetaData.getMappingVersion();
final long newMappingVersion = newIndexMetaData.getMappingVersion();
assert currentMappingVersion < newMappingVersion :
"expected current mapping version [" + currentMappingVersion + "] "
+ "to be less than new mapping version [" + newMappingVersion + "]";
assert updatedEntries.isEmpty() == false;
for (final DocumentMapper documentMapper : updatedEntries.values()) {
final MappingMetaData currentMapping = currentIndexMetaData.mapping(documentMapper.type());
if (currentMapping != null) {
final CompressedXContent currentSource = currentMapping.source();
final CompressedXContent newSource = documentMapper.mappingSource();
assert currentSource.equals(newSource) == false :
"expected current mapping [" + currentSource + "] for type [" + documentMapper.type() + "] " +
"to be different than new mapping";
}
}
}
}
}

public void merge(Map<String, Map<String, Object>> mappings, MergeReason reason) {
Map<String, CompressedXContent> mappingSourcesCompressed = new LinkedHashMap<>(mappings.size());
for (Map.Entry<String, Map<String, Object>> entry : mappings.entrySet()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ private void createIndices(final ClusterState state) {
AllocatedIndex<? extends Shard> indexService = null;
try {
indexService = indicesService.createIndex(indexMetaData, buildInIndexListener);
if (indexService.updateMapping(indexMetaData) && sendRefreshMapping) {
if (indexService.updateMapping(null, indexMetaData) && sendRefreshMapping) {
nodeMappingRefreshAction.nodeMappingRefresh(state.nodes().getMasterNode(),
new NodeMappingRefreshAction.NodeMappingRefreshRequest(indexMetaData.getIndex().getName(),
indexMetaData.getIndexUUID(), state.nodes().getLocalNodeId())
Expand Down Expand Up @@ -490,7 +490,7 @@ private void updateIndices(ClusterChangedEvent event) {
if (ClusterChangedEvent.indexMetaDataChanged(currentIndexMetaData, newIndexMetaData)) {
indexService.updateMetaData(newIndexMetaData);
try {
if (indexService.updateMapping(newIndexMetaData) && sendRefreshMapping) {
if (indexService.updateMapping(currentIndexMetaData, newIndexMetaData) && sendRefreshMapping) {
nodeMappingRefreshAction.nodeMappingRefresh(state.nodes().getMasterNode(),
new NodeMappingRefreshAction.NodeMappingRefreshRequest(newIndexMetaData.getIndex().getName(),
newIndexMetaData.getIndexUUID(), state.nodes().getLocalNodeId())
Expand Down Expand Up @@ -778,7 +778,7 @@ public interface AllocatedIndex<T extends Shard> extends Iterable<T>, IndexCompo
/**
* Checks if index requires refresh from master.
*/
boolean updateMapping(IndexMetaData indexMetaData) throws IOException;
boolean updateMapping(IndexMetaData currentIndexMetaData, IndexMetaData newIndexMetaData) throws IOException;

/**
* Returns shard with given id.
Expand Down
Loading