Skip to content

Dynamic mapping updates do not call fixRedundantIncludes #74899

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

Closed
romseygeek opened this issue Jul 5, 2021 · 4 comments · Fixed by #74903
Closed

Dynamic mapping updates do not call fixRedundantIncludes #74899

romseygeek opened this issue Jul 5, 2021 · 4 comments · Fixed by #74903
Labels
>bug :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v7.9.0

Comments

@romseygeek
Copy link
Contributor

RootObjectMapper has a method fixRedundantIncludes which will iterate
through its object hierarchy and find nested mappers that have include_in_root
set where they don't need it. However, this method is not called when a dynamic
mapping is built. This means that a bulk index request that creates a nested mapper
through a dynamic mapping update can end up throwing errors: the dynamic
mapping contains a nested object with both 'include_in_parent' and 'include_in_root'
set to true; this mapping is applied on the master and `include_in_root' is adjusted
to 'false' because it is redundant; the same dynamic mapping on a different shard
is sent to the master; and this mapping will fail because merging is done before
redundant includes are fixed, and so there is a conflict.

@romseygeek romseygeek added >bug :Search Foundations/Mapping Index mappings, including merging and defining field types v7.9.0 labels Jul 5, 2021
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jul 5, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@javanna
Copy link
Member

javanna commented Jul 5, 2021

We've talked in the past about making this validation step part of the creation of a new DocumentMapper. I was surprised that it is called only in some cases and I believe I left as-is, unless I introduced bugs while touching that area of the code. It sounds like it should be made part of creating a new mapping regardless of how it is created.

@javanna
Copy link
Member

javanna commented Jul 5, 2021

From my notes:

Validate DocumentMapper as part of its construction (by calling fixRedundantIncludes and validate): it seems like there are cases where we create it without validating it, is that a bug or a feature?

@romseygeek
Copy link
Contributor Author

Yeah I think this will be fixed by #74410 which makes it part of the build/merge logic for NestedObjectMapper itself but I'll do a separate fix for 7.14/7.13.4 anyway and we can re-use the tests.

romseygeek added a commit that referenced this issue Jul 5, 2021
Dynamic mappings updates containing nested mappings were not fixing
their redundant include settings before being posted back to the put
mappings service; this meant that a dynamic mapping that had both
'include_in_parent' and 'include_in_root' set to 'true' could cause an
error if it was returned from multiple shards, because the first mapping
would be updated to have 'include_in_root' set to 'false' when merged,
and merging this with the second mapping with 'include_in_root' set to
'true' would cause a conflict.

This commit ensures that fixRedundantIncludes is called on dynamic
mappings before they are posted back to the mapping service.

Fixes #74899
elasticsearchmachine pushed a commit to elasticsearchmachine/elasticsearch that referenced this issue Jul 5, 2021
Dynamic mappings updates containing nested mappings were not fixing
their redundant include settings before being posted back to the put
mappings service; this meant that a dynamic mapping that had both
'include_in_parent' and 'include_in_root' set to 'true' could cause an
error if it was returned from multiple shards, because the first mapping
would be updated to have 'include_in_root' set to 'false' when merged,
and merging this with the second mapping with 'include_in_root' set to
'true' would cause a conflict.

This commit ensures that fixRedundantIncludes is called on dynamic
mappings before they are posted back to the mapping service.

Fixes elastic#74899
elasticsearchmachine pushed a commit to elasticsearchmachine/elasticsearch that referenced this issue Jul 5, 2021
Dynamic mappings updates containing nested mappings were not fixing
their redundant include settings before being posted back to the put
mappings service; this meant that a dynamic mapping that had both
'include_in_parent' and 'include_in_root' set to 'true' could cause an
error if it was returned from multiple shards, because the first mapping
would be updated to have 'include_in_root' set to 'false' when merged,
and merging this with the second mapping with 'include_in_root' set to
'true' would cause a conflict.

This commit ensures that fixRedundantIncludes is called on dynamic
mappings before they are posted back to the mapping service.

Fixes elastic#74899
romseygeek pushed a commit that referenced this issue Jul 5, 2021
…74918)

Dynamic mappings updates containing nested mappings were not fixing
their redundant include settings before being posted back to the put
mappings service; this meant that a dynamic mapping that had both
'include_in_parent' and 'include_in_root' set to 'true' could cause an
error if it was returned from multiple shards, because the first mapping
would be updated to have 'include_in_root' set to 'false' when merged,
and merging this with the second mapping with 'include_in_root' set to
'true' would cause a conflict.

This commit ensures that fixRedundantIncludes is called on dynamic
mappings before they are posted back to the mapping service.

Fixes #74899
romseygeek pushed a commit that referenced this issue Jul 5, 2021
…74918)

Dynamic mappings updates containing nested mappings were not fixing
their redundant include settings before being posted back to the put
mappings service; this meant that a dynamic mapping that had both
'include_in_parent' and 'include_in_root' set to 'true' could cause an
error if it was returned from multiple shards, because the first mapping
would be updated to have 'include_in_root' set to 'false' when merged,
and merging this with the second mapping with 'include_in_root' set to
'true' would cause a conflict.

This commit ensures that fixRedundantIncludes is called on dynamic
mappings before they are posted back to the mapping service.

Fixes #74899
romseygeek pushed a commit that referenced this issue Jul 5, 2021
…74917)

Dynamic mappings updates containing nested mappings were not fixing
their redundant include settings before being posted back to the put
mappings service; this meant that a dynamic mapping that had both
'include_in_parent' and 'include_in_root' set to 'true' could cause an
error if it was returned from multiple shards, because the first mapping
would be updated to have 'include_in_root' set to 'false' when merged,
and merging this with the second mapping with 'include_in_root' set to
'true' would cause a conflict.

This commit ensures that fixRedundantIncludes is called on dynamic
mappings before they are posted back to the mapping service.

Fixes #74899
@javanna javanna added Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch and removed Team:Search Meta label for search team labels Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v7.9.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants