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 dynamic fields in mapping on master even if parsing fails for the rest of the doc #9874

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@brwe
Contributor

brwe commented Feb 25, 2015

...ils for the rest of doc

The local DocumentMapper is updated while parsing and dynamic fields are added before
parsing has finished. If parsing fails after a dynamic field has been added already
then the field was not added to the cluster state but was present in the local mapper of this
node. New documents with the same field would not necessarily cause an update either and
after restarting the node the mapping for these fields were lost. Instead the new fields
should always be updated.

closes #9851

@jpountz

View changes

Show outdated Hide outdated src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java
@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Mar 5, 2015

Contributor

LGTM

Contributor

jpountz commented Mar 5, 2015

LGTM

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Mar 5, 2015

Contributor

@brwe Since this change looks quite significant, I don't think it should be pushed to 1.4.5.

Contributor

jpountz commented Mar 5, 2015

@brwe Since this change looks quite significant, I don't think it should be pushed to 1.4.5.

@brwe brwe removed review v1.4.5 labels Mar 5, 2015

@brwe brwe closed this in d9a1540 Mar 6, 2015

brwe added a commit that referenced this pull request Mar 6, 2015

[mappings] update dynamic fields in mapping on master even if parsing…
… fails for the rest of doc

The local DocumentMapper is updated while parsing and dynamic fields are added before
parsing has finished. If parsing fails after a dynamic field has been added already
then the field was not added to the cluster state but was present in the local mapper of this
node. New documents with the same field would not necessarily cause an update either and
after restarting the node the mapping for these fields were lost. Instead the new fields
should always be updated.

closes #9851
closes #9874
@brwe

This comment has been minimized.

Show comment
Hide comment
@brwe

brwe Mar 6, 2015

Contributor

That was not a good idea - the change of MapperParsingException seemes to have screwed up bwc for Exception serialization, see for example http://build-us-00.elasticsearch.org/job/es_bwc_1x/8385/
I reverted the commit now.

Contributor

brwe commented Mar 6, 2015

That was not a good idea - the change of MapperParsingException seemes to have screwed up bwc for Exception serialization, see for example http://build-us-00.elasticsearch.org/job/es_bwc_1x/8385/
I reverted the commit now.

@brwe brwe reopened this Mar 6, 2015

[mappings] update dynamic fields in mapping on master even if parsing…
… fails for the rest of doc

The local DocumentMapper is updated while parsing and dynamic fields are added before
parsing has finished. If parsing fails after a dynamic field has been added already
then the field was not added to the cluster state but was present in the local mapper of this
node. New documents with the same field would not necessarily cause an update either and
after restarting the node the mapping for these fields were lost. Instead the new fields
should always be updated.

closes #9851
closes #9874
@brwe

This comment has been minimized.

Show comment
Hide comment
@brwe

brwe Mar 17, 2015

Contributor

Ok, new plan. Instead of changing the mapper parsing exception we actually can just get the context from the document mapper and ask if the mapping was changed - much easier and I cannot see any downside to it. @jpountz would you mind taking another look?

Contributor

brwe commented Mar 17, 2015

Ok, new plan. Instead of changing the mapper parsing exception we actually can just get the context from the document mapper and ask if the mapping was changed - much easier and I cannot see any downside to it. @jpountz would you mind taking another look?

@s1monw s1monw added v1.6.0 and removed v1.6.0 v1.5.0 labels Mar 17, 2015

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Mar 24, 2015

Contributor

I'm not too happy with getting back to the cache to check whether mappings have been modified, I liked the previous solution better. But on ther other hand, I don't have a better idea. Can we maybe just use this approach on 1.x and keep the previous approach on master?

Contributor

jpountz commented Mar 24, 2015

I'm not too happy with getting back to the cache to check whether mappings have been modified, I liked the previous solution better. But on ther other hand, I don't have a better idea. Can we maybe just use this approach on 1.x and keep the previous approach on master?

@brwe

This comment has been minimized.

Show comment
Hide comment
@brwe

brwe Mar 26, 2015

Contributor

Ok, I'll do that.

Contributor

brwe commented Mar 26, 2015

Ok, I'll do that.

brwe added a commit that referenced this pull request Mar 26, 2015

[mappings] update dynamic fields in mapping on master even if parsing…
… fails for the rest of doc

The local DocumentMapper is updated while parsing and dynamic fields are added before
parsing has finished. If parsing fails after a dynamic field has been added already
then the field was not added to the cluster state but was present in the local mapper of this
node. New documents with the same field would not necessarily cause an update either and
after restarting the node the mapping for these fields were lost. Instead the new fields
should always be updated.

closes #9851
closes #9874

@brwe brwe closed this in edb6319 Mar 26, 2015

@clintongormley clintongormley added the >bug label Apr 4, 2015

@clintongormley clintongormley changed the title from [mappings] update dynamic fields in mapping on master even if parsing fa... to Update dynamic fields in mapping on master even if parsing fails for the rest of the doc May 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment