Skip to content
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

Same code path for dynamic mappings updates and updates coming from the API. #10593

Merged
merged 1 commit into from Apr 16, 2015

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Apr 14, 2015

We have two completely different code paths for mappings updates, depending on
whether they come from the API or are guessed based on the parsed documents.
This commit makes dynamic mappings updates execute like updates from the API.

The only change in behaviour is that a document that fails parsing can not
modify mappings anymore (useful to prevent issues such as #9851). Other than
that, this change should be fairly transparent to users but working this way
opens doors to other changes such as validating dynamic mappings updates on the
master node (#8688).

The way it works internally is that Mapper.parse now returns a Mapper instead
of being void. The returned Mapper represents a mapping update that has been
performed in order to parse the document. Mappings updates are propagated
recursively back to the root mapper, and once parsing is finished, we check
that the mappings update can be applied, and either fail the parsing if the
update cannot be merged (eg. because of a concurrent mapping update from the
API) or merge the update into the mappings.

However not all mappings updates can be applied recursively, copy_to for
instance can add mappings at totally different places in the tree. Because of
it I added ParseContext.rootMapperUpdates which copy_to fills when the
field to copy data to does not exist in the mappings yet. These mappings
updates are merged from the ones generated by regular parsing.

One particular mapping update was the auto_boost setting on the all root
mapper. Being tricky to work on, I removed it in favour of search-time checks
that payloads have been indexed.

One interesting side-effect of the change is that concurrency on ObjectMapper
is greatly simplified since we do not have to care anymore about having
concurrent dynamic mappings and API updates.

Closes #9364

@jpountz jpountz added v2.0.0-beta1 review :Search/Mapping Index mappings, including merging and defining field types labels Apr 14, 2015
@@ -637,8 +665,41 @@ public void traverse(ObjectMapperListener listener) {
rootObjectMapper.traverse(listener);
}

private MergeContext mergeContext(MergeFlags mergeFlags) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rename to newMergeContext or createMergeContext?

@rjernst
Copy link
Member

rjernst commented Apr 15, 2015

@jpountz This is awesome! LGTM

@jpountz jpountz force-pushed the fix/dynamic_mappings_updates branch from 8a3cac3 to 38bec83 Compare April 16, 2015 07:43
…ing from the API.

We have two completely different code paths for mappings updates, depending on
whether they come from the API or are guessed based on the parsed documents.
This commit makes dynamic mappings updates execute like updates from the API.

The only change in behaviour is that a document that fails parsing can not
modify mappings anymore (useful to prevent issues such as elastic#9851). Other than
that, this change should be fairly transparent to users but working this way
opens doors to other changes such as validating dynamic mappings updates on the
master node (elastic#8688).

The way it works internally is that Mapper.parse now returns a Mapper instead
of being void. The returned Mapper represents a mapping update that has been
performed in order to parse the document. Mappings updates are propagated
recursively back to the root mapper, and once parsing is finished, we check
that the mappings update can be applied, and either fail the parsing if the
update cannot be merged (eg. because of a concurrent mapping update from the
API) or merge the update into the mappings.

However not all mappings updates can be applied recursively, `copy_to` for
instance can add mappings at totally different places in the tree. Because of
it I added ParseContext.rootMapperUpdates which `copy_to` fills when the
field to copy data to does not exist in the mappings yet. These mappings
updates are merged from the ones generated by regular parsing.

One particular mapping update was the `auto_boost` setting on the `all` root
mapper. Being tricky to work on, I removed it in favour of search-time checks
that payloads have been indexed.

One interesting side-effect of the change is that concurrency on ObjectMapper
is greatly simplified since we do not have to care anymore about having
concurrent dynamic mappings and API updates.
@jpountz jpountz force-pushed the fix/dynamic_mappings_updates branch from 38bec83 to 563e704 Compare April 16, 2015 08:17
@jpountz jpountz removed the review label Apr 16, 2015
jpountz added a commit that referenced this pull request Apr 16, 2015
Mappings: Same code path for dynamic mappings updates and updates coming from the API.

Close #10593
@jpountz jpountz merged commit 5806e85 into elastic:master Apr 16, 2015
@jpountz jpountz deleted the fix/dynamic_mappings_updates branch April 16, 2015 08:17
@jpountz
Copy link
Contributor Author

jpountz commented Apr 16, 2015

Thanks @rjernst !

jpountz added a commit to jpountz/elasticsearch that referenced this pull request Apr 22, 2015
While dynamic mappings updates are using the same code path as updates from the
API when applied on a data node since elastic#10593, they were still using a different
code path on the master node. This commit makes dynamic updates processed the
same way as updates from the API, which also seems to do a better way at
acknowledgements (I could not reproduce the ConcurrentDynamicTemplateTests
failure anymore). It also adds more checks, like for instance that indexing on
replicas should not trigger dynamic mapping updates since they should have been
handled on the primary before.
jpountz added a commit to jpountz/elasticsearch that referenced this pull request Apr 23, 2015
While dynamic mappings updates are using the same code path as updates from the
API when applied on a data node since elastic#10593, they were still using a different
code path on the master node. This commit makes dynamic updates processed the
same way as updates from the API, which also seems to do a better way at
acknowledgements (I could not reproduce the ConcurrentDynamicTemplateTests
failure anymore). It also adds more checks, like for instance that indexing on
replicas should not trigger dynamic mapping updates since they should have been
handled on the primary before.

Close elastic#10720
@clintongormley clintongormley changed the title Mappings: Same code path for dynamic mappings updates and updates coming from the API. Same code path for dynamic mappings updates and updates coming from the API. Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Mapping Index mappings, including merging and defining field types v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mappings: dynamic mapping updates should use the same path as mapping updates from the API
4 participants