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

Mappings: Update mapping on master in async manner #6648

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@kimchy
Member

kimchy commented Jun 30, 2014

Today, when a new mapping is introduced, the mapping is rebuilt (refreshSource) on the thread that performs the indexing request. This can become heavier and heavier if new mappings keeps on being introduced, we can move this process to another thread that will be responsible to refresh the source and then send the update mapping to the master (note, this doesn't change the semantics of new mapping introduction, since they are async anyhow).
When doing so, the thread can also try and batch as much updates as possible, this is handy especially when multiple shards for the same index exists on the same node. An internal setting that can control the time to wait for batches is also added (defaults to 0).

Testing wise, a new support method on ElasticsearchIntegrationTest#waitForConcreteMappingsOnAll to allow to wait for the concrete manifestation of mappings on all relevant nodes is added. Some tests mistakenly rely on the fact that there are no more pending tasks to mean mappings have been updated, so if we see, timing related, failures down later (all tests pass), then those will need to be fixed to wither awaitBusy on the master for the new mapping, or in the rare case, wait for the concrete mapping on all the nodes using the new method.

Note, this change also removes action.wait_on_mapping_change, this is an internal setting, and is not recommended to set it. It was used using the old test infrastructure to validate if the problem was due to mapping propagation, but we have a much better infra for this now.

* and sent to master for heavy single index requests that each introduce a new mapping, and when
* multiple shards exists on the same nodes, allowing to work on the index level in this case.
*/
private class MasterMappingUpdater extends Thread {

This comment has been minimized.

@martijnvg

martijnvg Jun 30, 2014

Member

Can we implement Runnable instead of extending from Thread?

This comment has been minimized.

@kimchy

kimchy Jun 30, 2014

Member

I thought about it, but then we need to have the runnable around, with the thread as variables. I like this encapsulation, I don't think extending Thread is such a bad idea for this case. Will switch if there is strong sentiment about it

@martijnvg

View changes

src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java Outdated
@@ -65,63 +73,33 @@ public MappingUpdatedAction(Settings settings, TransportService transportService
super(settings, transportService, clusterService, threadPool);
this.metaDataMappingService = metaDataMappingService;
this.indicesService = indicesService;
this.waitForMappingChange = settings.getAsBoolean("action.wait_on_mapping_change", false);
// this setting should probably always be 0, just add the option to wait for more changes within a time window
TimeValue additionalMappingChangeTime = settings.getAsTime("action.mapping.additional_mapping_change_time", TimeValue.timeValueMillis(0));

This comment has been minimized.

@martijnvg

martijnvg Jun 30, 2014

Member

Maybe this can be a setting that is updateable at runtime? If many mapping updates become a problem then this setting can be increased without restarting nodes.

This comment has been minimized.

@kimchy

kimchy Jun 30, 2014

Member

added

@martijnvg

This comment has been minimized.

Member

martijnvg commented Jun 30, 2014

Left two comments, this change looks good to me. Maybe someone else can also take a look?

@kimchy kimchy added review labels Jun 30, 2014

@bleskes

View changes

src/main/java/org/elasticsearch/node/internal/InternalNode.java Outdated
@@ -323,6 +324,8 @@ public void close() {
stopWatch.stop().start("rivers");
injector.getInstance(RiversManager.class).close();
stopWatch.stop().start("mapping");
injector.getInstance(MappingUpdatedAction.class).close();

This comment has been minimized.

@bleskes

bleskes Jun 30, 2014

Member

I think it's safer to introduce a stop() method and call it in the stop phase as the masterMappingUpdater thread might try to speak to the cluster service which has been stopped, leading to errors & warning logs

}
List<MappingChange> changes = Lists.newArrayList(polledChange);
if (additionalMappingChangeTime.millis() > 0) {
Thread.sleep(additionalMappingChangeTime.millis());

This comment has been minimized.

@bleskes

bleskes Jun 30, 2014

Member

I think we should catch and ignore interrupt messages here to reduce the chance we loose a mapping update?

This comment has been minimized.

@kimchy

kimchy Jun 30, 2014

Member

the outer catch interrupt should handle it I think..., if someone interrupts this thread, its only when its shutting down, I can add a check and log if its still in running state

This comment has been minimized.

@bleskes

bleskes Jun 30, 2014

Member

What I mean is that if we sleep here, we know for sure that there is a change event that should have gone to the master. I we just let it flow through the code, it will first send it and then check running and stop.

@bleskes

View changes

src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java Outdated
@@ -180,7 +180,7 @@ protected ShardIterator shards(ClusterState clusterState, BulkShardRequest reque
applyVersion(request.items()[j], preVersions[j], preVersionTypes[j]);
}
for (Tuple<String, String> mappingToUpdate : mappingsToUpdate) {
mappingUpdatedAction.updateMappingOnMaster(mappingToUpdate.v1(), mappingToUpdate.v2(), true);
mappingUpdatedAction.updateMappingOnMaster(mappingToUpdate.v1(), mappingToUpdate.v2());

This comment has been minimized.

@bleskes

bleskes Jun 30, 2014

Member

unrelated to the change, but we should add the index uuid here. seems small enough to inline with this change?

@bleskes

View changes

src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java Outdated
@@ -340,7 +340,7 @@ protected ShardIterator shards(ClusterState clusterState, BulkShardRequest reque
}
for (Tuple<String, String> mappingToUpdate : mappingsToUpdate) {
mappingUpdatedAction.updateMappingOnMaster(mappingToUpdate.v1(), mappingToUpdate.v2(), true);
mappingUpdatedAction.updateMappingOnMaster(mappingToUpdate.v1(), mappingToUpdate.v2());

This comment has been minimized.

@bleskes

bleskes Jun 30, 2014

Member

same comment about index uuid

@bleskes

View changes

src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java Outdated
nodeNames.add(node.getName());
}
// get the nodes info to get the node names...
NodesInfoResponse nodes = client().admin().cluster().prepareNodesInfo(nodeNames.toArray(new String[nodeNames.size()])).get();

This comment has been minimized.

@bleskes

bleskes Jun 30, 2014

Member

I don't think we need this as only use the node name which we already have

@bleskes

View changes

src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java Outdated
public boolean apply(Object input) {
try {
// make sure we only ask for nodes info names that hold a specific started shard (would be nice to have an ES API for it?)
ClusterStateResponse clusterState = client().admin().cluster().prepareState().setIndices(index).get();

This comment has been minimized.

@bleskes

bleskes Jun 30, 2014

Member

Consider using org.elasticsearch.test.InternalTestCluster#nodesInclude . It currently gives back a list of all the nodes which should have a shard of the index (started or not), but I think it's maybe better to actually validate the shard that were intializing also end up with the right mapping at the end?

@bleskes

This comment has been minimized.

Member

bleskes commented Jun 30, 2014

I went through the change. Bulk of it looks good. Left some minor comments. I also wonder if we should mark it as breaking because we removed the action.wait_on_mapping_change option.

@bleskes bleskes removed the review label Jun 30, 2014

@kimchy kimchy added the breaking label Jun 30, 2014

@kimchy

This comment has been minimized.

Member

kimchy commented Jun 30, 2014

@bleskes used the support method, added a note on breaking, also bit the bullet and cleaned all calls to update mapping to include doc mapper and UUID actually used

sortedMappers.put(cursor.key, cursor.value);
}
Mapper[] sortedMappers = mappers.values().toArray(Mapper.class);
Arrays.sort(sortedMappers, new Comparator<Mapper>() {

This comment has been minimized.

@bleskes

bleskes Jun 30, 2014

Member

cool. did this come out of profiling?

This comment has been minimized.

@kimchy
mappingUpdatedAction.updateMappingOnMaster(mappingToUpdate.v1(), mappingToUpdate.v2(), true);
DocumentMapper docMapper = indexService.mapperService().documentMapper(mappingToUpdate.v2());
if (docMapper != null) {
mappingUpdatedAction.updateMappingOnMaster(mappingToUpdate.v1(), docMapper, indexService.indexUUID());

This comment has been minimized.

@bleskes

bleskes Jun 30, 2014

Member

cool. thx

change.documentMapper.refreshSource();
DiscoveryNode node = clusterService.localNode();
mappingRequest = new MappingUpdatedAction.MappingUpdatedRequest(
change.index, change.indexUUID, change.documentMapper.type(), change.documentMapper.mappingSource(), orderId, node != null ? node.id() : null

This comment has been minimized.

@bleskes

bleskes Jun 30, 2014

Member

I have a concern regarding batching and the fact that we use the first change object. Since we don't directly access the MappingService we might fail to send a newer version of the doc. I like the change made to updateMappingOnMaster to only keep one variant. Maybe we should keep just one which refers to the type as string and always resolve the current doc mapper here.

This comment has been minimized.

@kimchy

kimchy Jun 30, 2014

Member

I wasn't that concerned about it, but to be on the safe side, I reversed the order of the events

int numDocs = randomIntBetween(100, 1000);
int numberOfFields = randomIntBetween(1, 50);
Set<Integer> fieldsIdx = Sets.newHashSet();
IndexRequestBuilder[] builders = new IndexRequestBuilder[numDocs];

This comment has been minimized.

@bleskes

bleskes Jun 30, 2014

Member

I think we should randomly set indices.mapping.additional_mapping_change_time to make sure that works too.

This comment has been minimized.

@kimchy

kimchy Jun 30, 2014

Member

I would like to add this randomized through all our test infra, but on a different change

.field(Loading.KEY, randomFrom(Loading.LAZY, Loading.EAGER))
.endObject()
.endObject()
.endObject()

This comment has been minimized.

@bleskes

bleskes Jun 30, 2014

Member

How I wish we could tell Intellij not to do that...

This comment has been minimized.

@nik9000

nik9000 Jun 30, 2014

Contributor

In Eclipse there is a special comment you can put that stops the formatter. Its ugly, only works in Eclipse, and isn't on by default. I just try to be very careful.

This comment has been minimized.

@pickypg

pickypg Jun 30, 2014

Member

A couple of useful IntelliJ settings that should help to avoid it annoyingly reformatting code as you go:

  • Settings -> Editor -> Smart Keys -> Reformat block on typing '}' [Uncheck]
  • Settings -> Editor -> Smart Keys -> Reformat on Paste [None]

Both of those are enabled by default, which I have always found to be quite annoying. I believe the first one was added in version 13, but it has been too long to remember.

@bleskes

This comment has been minimized.

Member

bleskes commented Jun 30, 2014

+1

kimchy added some commits Jun 30, 2014

Update mapping on master in async manner
Today, when a new mapping is introduced, the mapping is rebuilt (refreshSource) on the thread that performs the indexing request. This can become heavier and heavier if new mappings keeps on being introduced, we can move this process to another thread that will be responsible to refresh the source and then send the update mapping to the master (note, this doesn't change the semantics of new mapping introduction, since they are async anyhow).
When doing so, the thread can also try and batch as much updates as possible, this is handy especially when multiple shards for the same index exists on the same node. An internal setting that can control the time to wait for batches is also added (defaults to 0).

Testing wise, a new support method on ElasticsearchIntegrationTest#waitForConcreteMappingsOnAll to allow to wait for the concrete manifestation of mappings on all relevant nodes is added. Some tests mistakenly rely on the fact that there are no more pending tasks to mean mappings have been updated, so if we see, timing related, failures down later (all tests pass), then those will need to be fixed to wither awaitBusy on the master for the new mapping, or in the rare case, wait for the concrete mapping on all the nodes using the new method.
closes #6648
better sorting on mappers when refreshing source
also, no need to call nodes info in test, we already have the node names
clean calls to mapping update to provide doc mapper and UUID always
also use the internal cluster support method to get the list of nodes an index is on

@kimchy kimchy closed this in 5273410 Jun 30, 2014

kimchy added a commit that referenced this pull request Jun 30, 2014

Update mapping on master in async manner
Today, when a new mapping is introduced, the mapping is rebuilt (refreshSource) on the thread that performs the indexing request. This can become heavier and heavier if new mappings keeps on being introduced, we can move this process to another thread that will be responsible to refresh the source and then send the update mapping to the master (note, this doesn't change the semantics of new mapping introduction, since they are async anyhow).
When doing so, the thread can also try and batch as much updates as possible, this is handy especially when multiple shards for the same index exists on the same node. An internal setting that can control the time to wait for batches is also added (defaults to 0).

Testing wise, a new support method on ElasticsearchIntegrationTest#waitForConcreteMappingsOnAll to allow to wait for the concrete manifestation of mappings on all relevant nodes is added. Some tests mistakenly rely on the fact that there are no more pending tasks to mean mappings have been updated, so if we see, timing related, failures down later (all tests pass), then those will need to be fixed to wither awaitBusy on the master for the new mapping, or in the rare case, wait for the concrete mapping on all the nodes using the new method.
closes #6648

@kimchy kimchy deleted the kimchy:update_mapping_master_async branch Jun 30, 2014

bleskes added a commit to bleskes/elasticsearch that referenced this pull request Jul 7, 2014

[Relocation] process pending mapping update in phase 2
During phase1 we copy over all lucene segments. These make refer to mapping updates that are still queued up to be sent to master. We must make sure those pending updates are sent before completing the relocation.

Relates to elastic#6648

bleskes added a commit that referenced this pull request Jul 7, 2014

[Relocation] process pending mapping update in phase 2
During phase1 we copy over all lucene segments. These may refer to mapping updates that are still queued up to be sent to master. We must make sure those pending updates are processed before completing the relocation.

Relates to #6648

Closes #6762

bleskes added a commit that referenced this pull request Jul 7, 2014

[Relocation] process pending mapping update in phase 2
During phase1 we copy over all lucene segments. These may refer to mapping updates that are still queued up to be sent to master. We must make sure those pending updates are processed before completing the relocation.

Relates to #6648

Closes #6762

@clintongormley clintongormley changed the title from Update mapping on master in async manner to Mappings: Update mapping on master in async manner Jul 16, 2014

im-denisenko added a commit to im-denisenko/Elastica that referenced this pull request Jan 5, 2015

Remove ES_WAIT_ON_MAPPING_CHANGE from travis
Config option "action.wait_on_mapping_change" was removed since ES 1.3.0
elastic/elasticsearch#6648

im-denisenko added a commit to im-denisenko/Elastica that referenced this pull request Jan 5, 2015

Remove ES_WAIT_ON_MAPPING_CHANGE from travis
Config option "action.wait_on_mapping_change" was removed since ES 1.3.0
elastic/elasticsearch#6648
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment