elastic / elasticsearch

Free and Open Source, Distributed, RESTful Search Engine
https://www.elastic.co/products/elasticsearch
Other
69.95k stars 24.74k forks source link

Mapping exception due to race condition with dynamic mappings #114228

Open felixbarny opened 2 weeks ago

felixbarny commented 2 weeks ago

There's a race condition when concurrently indexing documents with different dynamic types.

For example, when concurrently indexing two documents with the values 0 and 0.1, ES will try to map the field to long and float, respectively. As that's happening concurrently, the pre-flight checks (MergeReason.MAPPING_AUTO_UPDATE_PREFLIGHT) in both threads will be successful, as both think that they'll add the initial mapping of the field. However, when both request the mapping update, the master node will run the update in a single thread and only one of them can "win". The request that is handled second will be rejected as changing the type of a field is not allowed (makes sense).

java.lang.IllegalArgumentException: mapper [field] cannot be changed from type [float] to [long]
        at __randomizedtesting.SeedInfo.seed([49E87AA320D5CE88:7067D21C0D45D7A6]:0)
        at org.elasticsearch.index.mapper.FieldMapper.checkIncomingMergeType(FieldMapper.java:420)
        at org.elasticsearch.index.mapper.FieldMapper.merge(FieldMapper.java:400)
        at org.elasticsearch.index.mapper.FieldMapper.merge(FieldMapper.java:63)
        at org.elasticsearch.index.mapper.ObjectMapper$MergeResult.buildMergedMappers(ObjectMapper.java:681)
        at org.elasticsearch.index.mapper.ObjectMapper$MergeResult.build(ObjectMapper.java:620)
        at org.elasticsearch.index.mapper.RootObjectMapper.merge(RootObjectMapper.java:234)
        at org.elasticsearch.index.mapper.Mapping.merge(Mapping.java:144)
        at org.elasticsearch.index.mapper.MapperService.mergeMappings(MapperService.java:641)
        at org.elasticsearch.index.mapper.MapperService.mergeMappings(MapperService.java:608)
        at org.elasticsearch.cluster.metadata.MetadataMappingService$PutMappingExecutor.applyRequest(MetadataMappingService.java:154)
        at org.elasticsearch.cluster.metadata.MetadataMappingService$PutMappingExecutor.execute(MetadataMappingService.java:117)
        at org.elasticsearch.cluster.service.MasterService.innerExecuteTasks(MasterService.java:1058)
        at org.elasticsearch.cluster.service.MasterService.executeTasks(MasterService.java:1021)
        at org.elasticsearch.cluster.service.MasterService.executeAndPublishBatch(MasterService.java:233)
        at org.elasticsearch.cluster.service.MasterService$BatchingTaskQueue$Processor.lambda$run$2(MasterService.java:1674)
        at org.elasticsearch.action.ActionListener.run(ActionListener.java:452)
        at org.elasticsearch.cluster.service.MasterService$BatchingTaskQueue$Processor.run(MasterService.java:1671)
        at org.elasticsearch.cluster.service.MasterService$5.lambda$doRun$0(MasterService.java:1266)
        at org.elasticsearch.action.ActionListener.run(ActionListener.java:452)
        at org.elasticsearch.cluster.service.MasterService$5.doRun(MasterService.java:1245)
        at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:992)
        at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:27)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
        at java.lang.Thread.run(Thread.java:1570)

I'm proposing two different options that mitigate the exception:

elasticsearchmachine commented 2 weeks ago

Pinging @elastic/es-storage-engine (Team:StorageEngine)

elasticsearchmachine commented 2 weeks ago

Pinging @elastic/es-distributed (Team:Distributed)

kkrik-es commented 2 weeks ago

Do I understand correctly that, regardless, one of the updates will "win" in the master? E.g. if the "long" mapping get applied first at the master, all subsequent documents with float values will be tracked as malformed. If this holds, the aforementioned solutions would just save us from a single indexing failures, with potentially thousands or millions of docs following with malformed entries?

If this is the case, I wonder if it's preferable to avoid the extra complexity and just rely on failure store to track the dropped doc. As a bonus, we have a record of the indexing failure that can help identify the problem and fix the mapping.

felixbarny commented 2 weeks ago

Yes, only one update will win. Whether or not indexing will fail depends on a couple of factors: ES will try to coerce the values. In the Long/float example, coercing will always work (although there will be precision loss). If the value cant be coerced, ignore_malformed can prevent an indexing failure.

Another concrete case that we‘ve seen is inconsistent definitions of http.status_code. Some instrumentations send it as a String and others as a number. No matter which Definition wins, the value can always be coerced, so that there will be no indexing issues.

That aside, this is a genuine concurrency bug in ES. When applying the same set of instructions serially, this exception doesnt occurr.

elasticsearchmachine commented 2 weeks ago

Pinging @elastic/es-search-foundations (Team:Search Foundations)

felixbarny commented 2 weeks ago

Sorry, I just realized that I have probably triaged the issue to the wrong team. It should probably be :Search Foundations/Mapping instead of :StorageEngine/Mapping, as this is about the mapper merge logic.

henningandersen commented 2 weeks ago

If we can make #114227 work, that would be my preference, since the master action will then succeed (the field was indeed added) and on the retry, the indexing request should then succeed or fail as appropriate. Failing the dynamic mapping update master action and trying to handle that later will require us to know more about the exception/situation in order to retry. To some extent, the dynamic mapping update did succeed too, the fact that there was a race should be internal to the master.