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 @@ -292,6 +292,7 @@ public ClusterState execute(ClusterState currentState) {
// Index exists and it's closed - open it in metadata and start recovery
IndexMetaData.Builder indexMdBuilder = IndexMetaData.builder(snapshotIndexMetaData).state(IndexMetaData.State.OPEN);
indexMdBuilder.version(Math.max(snapshotIndexMetaData.getVersion(), currentIndexMetaData.getVersion() + 1));
indexMdBuilder.mappingVersion(Math.max(snapshotIndexMetaData.getMappingVersion(), currentIndexMetaData.getMappingVersion() + 1));
if (!request.includeAliases()) {
// Remove all snapshot aliases
if (!snapshotIndexMetaData.getAliases().isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@

import org.elasticsearch.action.admin.indices.mapping.put.PutMappingClusterStateUpdateRequest;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.ClusterStateTaskExecutor;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.IndexService;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.test.ESSingleNodeTestCase;
Expand All @@ -47,7 +49,8 @@ public void testMappingClusterStateUpdateDoesntChangeExistingIndices() throws Ex
final ClusterService clusterService = getInstanceFromNode(ClusterService.class);
// TODO - it will be nice to get a random mapping generator
final PutMappingClusterStateUpdateRequest request = new PutMappingClusterStateUpdateRequest().type("type");
request.source("{ \"properties\" { \"field\": { \"type\": \"text\" }}}");
request.indices(new Index[] {indexService.index()});
request.source("{ \"properties\": { \"field\": { \"type\": \"text\" }}}");
mappingService.putMappingExecutor.execute(clusterService.state(), Collections.singletonList(request));
assertThat(indexService.mapperService().documentMapper("type").mappingSource(), equalTo(currentMapping));
}
Expand All @@ -69,4 +72,35 @@ public void testClusterStateIsNotChangedWithIdenticalMappings() throws Exception

assertSame(result, result2);
}

public void testMappingVersion() throws Exception {
final IndexService indexService = createIndex("test", client().admin().indices().prepareCreate("test").addMapping("type"));
final long previousVersion = indexService.getMetaData().getMappingVersion();
final MetaDataMappingService mappingService = getInstanceFromNode(MetaDataMappingService.class);
final ClusterService clusterService = getInstanceFromNode(ClusterService.class);
final PutMappingClusterStateUpdateRequest request = new PutMappingClusterStateUpdateRequest().type("type");
request.indices(new Index[] {indexService.index()});
request.source("{ \"properties\": { \"field\": { \"type\": \"text\" }}}");
final ClusterStateTaskExecutor.ClusterTasksResult<PutMappingClusterStateUpdateRequest> result =
mappingService.putMappingExecutor.execute(clusterService.state(), Collections.singletonList(request));
assertThat(result.executionResults.size(), equalTo(1));
assertTrue(result.executionResults.values().iterator().next().isSuccess());
assertThat(result.resultingState.metaData().index("test").getMappingVersion(), equalTo(1 + previousVersion));
}

public void testMappingVersionUnchanged() throws Exception {
final IndexService indexService = createIndex("test", client().admin().indices().prepareCreate("test").addMapping("type"));
final long previousVersion = indexService.getMetaData().getMappingVersion();
final MetaDataMappingService mappingService = getInstanceFromNode(MetaDataMappingService.class);
final ClusterService clusterService = getInstanceFromNode(ClusterService.class);
final PutMappingClusterStateUpdateRequest request = new PutMappingClusterStateUpdateRequest().type("type");
request.indices(new Index[] {indexService.index()});
request.source("{ \"properties\": {}}");
final ClusterStateTaskExecutor.ClusterTasksResult<PutMappingClusterStateUpdateRequest> result =
mappingService.putMappingExecutor.execute(clusterService.state(), Collections.singletonList(request));
assertThat(result.executionResults.size(), equalTo(1));
assertTrue(result.executionResults.values().iterator().next().isSuccess());
assertThat(result.resultingState.metaData().index("test").getMappingVersion(), equalTo(previousVersion));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ public void testLoadState() throws IOException {
IndexMetaData deserialized = indices.get(original.getIndex().getName());
assertThat(deserialized, notNullValue());
assertThat(deserialized.getVersion(), equalTo(original.getVersion()));
assertThat(deserialized.getMappingVersion(), equalTo(original.getMappingVersion()));
assertThat(deserialized.getNumberOfReplicas(), equalTo(original.getNumberOfReplicas()));
assertThat(deserialized.getNumberOfShards(), equalTo(original.getNumberOfShards()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.elasticsearch.Version;
import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsResponse;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.compress.CompressedXContent;
Expand Down Expand Up @@ -741,4 +742,13 @@ public void testDynamicTemplateOrder() throws IOException {
client().prepareIndex("test", "type", "1").setSource("foo", "abc").get();
assertThat(index.mapperService().fullName("foo"), instanceOf(KeywordFieldMapper.KeywordFieldType.class));
}

public void testMappingVersionAfterDynamicMappingUpdate() {
createIndex("test", client().admin().indices().prepareCreate("test").addMapping("type"));
final ClusterService clusterService = getInstanceFromNode(ClusterService.class);
final long previousVersion = clusterService.state().metaData().index("test").getMappingVersion();
client().prepareIndex("test", "type", "1").setSource("field", "text").get();
assertThat(clusterService.state().metaData().index("test").getMappingVersion(), equalTo(1 + previousVersion));
}

}
Loading