Update cluster state with type mapping also for failed indexing request #8692

brwe · 2014-11-27T17:54:24Z

When indexing of a document with a type that is not in the mappings fails,
for example because "dynamic": "strict" but doc contains a new field,
then the type is still created on the node that executed the indexing request.
However, the change was never added to the cluster state.
This commit makes sure mapping updates are always added to the cluster state
even if indexing of a document fails.

closes #8650

rjernst · 2014-12-02T22:15:54Z

src/test/java/org/elasticsearch/search/aggregations/bucket/DedicatedAggregationTests.java

+        client().admin().indices().prepareCreate("index").addMapping("_default_", defaultMapping).get();
+
+        try {
+            client().prepareIndex("index", "type", "id").setSource("{\"test\":\"test\"}").get();


You can just use setSource("test", "test")?

…xing request When indexing of a document with a type that is not in the mappings fails, for example because "dynamic": "strict" but doc contains a new field, then the type is still created on the node that executed the indexing request. However, the change was never added to the cluster state. This commit makes sure mapping updates are always added to the cluster state even if indexing of a document fails. closes elastic#8650

brwe · 2015-02-23T17:53:12Z

Updated. I also moved the integration test to a separate class as DedicatedAggregationTests is hardly the right place.

rjernst · 2015-02-23T22:44:44Z

src/main/java/org/elasticsearch/action/WriteFailure.java

+
+
+public class WriteFailure extends ElasticsearchException implements ElasticsearchWrapperException {
+    @Nullable


Why is this nullable when there is an assert forbidding null in the ctor?

ignore, im blind. :)

rjernst · 2015-02-23T23:06:21Z

@brwe I started looking at this again, but then realized I'm not sure this is what we want. Isn't the point of dynamic: strict to not modify mappings? Adding an empty type in this case seems counter intuitive...

brwe · 2015-02-24T11:18:56Z

Isn't the point of dynamic: strict to not modify mappings?

dynamic: strict means that no fields may be added that were not defined before. The setting for strictly not adding pre defined types is "index.mapper.dynamic": false.

However, I agree it is weird that the mapping is created although the document is not indexed. We create mappings in some places where we then do not index, like index failures in bulk requests (see #5623) or when percolating a document that has a type which was not defined before.
The reason why we do this is that currently mappings are added to the node local DocumentMapper the moment we parse. This means that we have to update the mapping, regardless of if the document is indexed or not because else the mapping in cluster state and in DocumentMapper are out of sync and therefore the mapping is lost the moment we restart the node.
While reading the code I actually found another example where this causes problems: When we parse a document halfway and then get a parsing exception, the parsed fields that were added before are then also not added to the cluster state and can be lost when restarting the node. I added a test and fix to this pr.
Now, I think we have two options:

fix: Parsing of documents that are not indexed should not change the Document mapper or the mapping in the cluster state
push the fixes as is which would at least prevent the mappings to be lost and live with the fact that mappings are created even if indexing of the doc fails.

I would like to do the latter and work on the first in a different issue and pr because I could imagine this needs a little more work.

rjernst · 2015-02-24T14:43:00Z

I'm all for making the behavior consistent and predictable. What concerns me is the future, when mappings are cleaned up and immutable (#9365) so that we can fix all of these issues you mention and not modify mappings when failures occur. I do not want someone to then argue the current behavior needs to be maintained. So can we downplay this in the docs somehow? I don't want anyone to rely on this behavior, or even have the expectation of relying on the behavior.

rjernst · 2015-02-24T14:46:31Z

Obviously this PR has no doc changes. I guess what I mean is, can we maintain the fact that this behavior cannot be relied upon if someone comes along and says "what is the behavior and why isn't it documented?".

This question aside, the change LGTM.

brwe · 2015-02-24T15:14:21Z

ok, changed WriteFailure -> WriteFailureException. Will push in a minute to 1.4 1.x and master

brwe · 2015-02-24T15:53:47Z

spoke too soon. the second fix should also be in prepareIndex and not just prepareCreate. Also, I am not sure if it is sufficient to check if it was a MapperParsingException but also we should probably check if the mapping was actually modified before we update on master.

This reverts commit 86a58d5.

brwe · 2015-02-24T16:17:10Z

I'll push only the original fix (mapping lost with dynamic_strict...) and make a separate issue for the second one.

…xing request When indexing of a document with a type that is not in the mappings fails, for example because "dynamic": "strict" but doc contains a new field, then the type is still created on the node that executed the indexing request. However, the change was never added to the cluster state. This commit makes sure mapping updates are always added to the cluster state even if indexing of a document fails. closes #8692 relates to #8650

javanna · 2015-02-25T09:07:45Z

@brwe can you please label? :)

…xing request When indexing of a document with a type that is not in the mappings fails, for example because "dynamic": "strict" but doc contains a new field, then the type is still created on the node that executed the indexing request. However, the change was never added to the cluster state. This commit makes sure mapping updates are always added to the cluster state even if indexing of a document fails. closes elastic#8692 relates to elastic#8650

rjernst reviewed Dec 2, 2014
View reviewed changes

drewr force-pushed the master branch from dcc3da0 to 7c20a8a Compare February 20, 2015 16:48

brwe force-pushed the fix-type-mapping-update-on-indexing-failure branch from 0992e7f to 73845eb Compare February 23, 2015 17:48

brwe force-pushed the fix-type-mapping-update-on-indexing-failure branch from 73845eb to 299abe1 Compare February 23, 2015 17:50

brwe added v2.0.0-beta1 review v1.5.0 v1.4.5 and removed v1.4.5 v1.5.0 v2.0.0-beta1 labels Feb 23, 2015

rjernst reviewed Feb 23, 2015
View reviewed changes

brwe added 2 commits February 24, 2015 11:53

another test and fix for partial parsed docs

86a58d5

remve unused var

4c16bcb

rename WriteFailure -> WriteFailureException

81a307d

Revert "another test and fix for partial parsed docs"

da34c48

This reverts commit 86a58d5.

brwe closed this in ff8fd67 Feb 24, 2015

brwe added v2.0.0-beta1 v1.5.0 :Search/Mapping Index mappings, including merging and defining field types v1.4.5 and removed review labels Feb 25, 2015

clintongormley added the >bug label Mar 19, 2015

clintongormley changed the title ~~mappings: update cluster state with type mapping also for failed indexin...~~ Update cluster state with type mapping also for failed indexing request Jun 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update cluster state with type mapping also for failed indexing request #8692

Update cluster state with type mapping also for failed indexing request #8692

brwe commented Nov 27, 2014

rjernst Dec 2, 2014

brwe commented Feb 23, 2015

rjernst Feb 23, 2015

rjernst Feb 23, 2015

rjernst commented Feb 23, 2015

brwe commented Feb 24, 2015

rjernst commented Feb 24, 2015

rjernst commented Feb 24, 2015

brwe commented Feb 24, 2015

brwe commented Feb 24, 2015

brwe commented Feb 24, 2015

javanna commented Feb 25, 2015



		public class WriteFailure extends ElasticsearchException implements ElasticsearchWrapperException {
		@Nullable

Update cluster state with type mapping also for failed indexing request #8692

Update cluster state with type mapping also for failed indexing request #8692

Conversation

brwe commented Nov 27, 2014

rjernst Dec 2, 2014

Choose a reason for hiding this comment

brwe commented Feb 23, 2015

rjernst Feb 23, 2015

Choose a reason for hiding this comment

rjernst Feb 23, 2015

Choose a reason for hiding this comment

rjernst commented Feb 23, 2015

brwe commented Feb 24, 2015

rjernst commented Feb 24, 2015

rjernst commented Feb 24, 2015

brwe commented Feb 24, 2015

brwe commented Feb 24, 2015

brwe commented Feb 24, 2015

javanna commented Feb 25, 2015