-
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
Same code path for dynamic mappings updates and updates coming from the API. #10593
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jpountz
added
v2.0.0-beta1
review
:Search Foundations/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) { |
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.
maybe rename to newMergeContext
or createMergeContext
?
@jpountz This is awesome! LGTM |
jpountz
force-pushed
the
fix/dynamic_mappings_updates
branch
from
April 16, 2015 07:43
8a3cac3
to
38bec83
Compare
…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
force-pushed
the
fix/dynamic_mappings_updates
branch
from
April 16, 2015 08:17
38bec83
to
563e704
Compare
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
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
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 Foundations/Mapping
Index mappings, including merging and defining field types
v2.0.0-beta1
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
forinstance can add mappings at totally different places in the tree. Because of
it I added ParseContext.rootMapperUpdates which
copy_to
fills when thefield 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 theall
rootmapper. 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