-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
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
client().admin().indices().prepareCreate("index").addMapping("_default_", defaultMapping).get(); | ||
|
||
try { | ||
client().prepareIndex("index", "type", "id").setSource("{\"test\":\"test\"}").get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just use setSource("test", "test")
?
0992e7f
to
73845eb
Compare
…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
73845eb
to
299abe1
Compare
Updated. I also moved the integration test to a separate class as DedicatedAggregationTests is hardly the right place. |
|
||
|
||
public class WriteFailure extends ElasticsearchException implements ElasticsearchWrapperException { | ||
@Nullable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this nullable when there is an assert forbidding null in the ctor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore, im blind. :)
@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... |
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.
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. |
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. |
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. |
ok, changed WriteFailure -> WriteFailureException. Will push in a minute to 1.4 1.x and master |
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.
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
…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
@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
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