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

Remove the _default_ mapping. #28248

Closed

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Jan 16, 2018

With only one type, the default mapping has become useless. The approach in
this commit is to reject mapping updates that try to update the _default
type regardless of the index creation version, and to ignore _default_
mappings when recovering indices from a 6.x index. In the corner case that
an empty 6.x index needs to be upgraded and this index only has a
_default_ mapping, then the _default_ mapping will be renamed to _doc,
which is the hardcoded mapping name in 7.0.

I need reviews especially on the cluster-state processing side of the PR to know
whether the approach I took in order to dynamically remove default mappings
when processing mappings is ok.

@jpountz jpountz added >breaking :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0 labels Jan 16, 2018
@jpountz jpountz force-pushed the enhancement/remove_default_mapping branch from 8171a26 to f6989ba Compare January 18, 2018 09:42
With only one type, the default mapping has become useless. The approach in
this commit is to reject mapping updates that try to update the `_default`
type regardless of the index creation version, and to ignore `_default_`
mappings when recovering indices from a 6.x index. In the corner case that
an empty 6.x index needs to be upgraded and this index only has a
`_default_` mapping, then the `_default_` mapping will be renamed to `_doc`,
which is the hardcoded mapping name in 7.0.
@jpountz jpountz force-pushed the enhancement/remove_default_mapping branch from dfe8ec0 to 35ce44c Compare January 22, 2018 11:31
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM

// new types all at once , which can create a false error.

// For example in MapperService we can't distinguish between a create index api call
// and a put mapping api call, so we don't which type did exist before.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/we don't/we don't know/

@jpountz jpountz assigned ywelsch and unassigned ywelsch Jan 24, 2018
@jpountz jpountz requested a review from ywelsch January 24, 2018 13:24
@ywelsch
Copy link
Contributor

ywelsch commented Feb 6, 2018

I find the approach chosen here to patch the cluster state problematic, as it leads to inconsistent views across 6.x / 7.x nodes. My suggestion is therefore to not fully remove default in 7.x just yet:

  • Keep default in the cluster state on 7.x (but add assertion that it only exists for 6.x indices)
  • Reject adding default mappings for 7.x indices (only single mapping in 7.x indices)
  • Keep MapperService etc. aware of default mappings because of 6.x indices
  • Remove everything in 8.x

jpountz added a commit to jpountz/elasticsearch that referenced this pull request Mar 20, 2018
This will reject mapping updates to the `_default_` mapping with 7.x indices
and still emit a deprecation warning with 6.x indices.

Relates elastic#15613
Supersedes elastic#28248
@jpountz
Copy link
Contributor Author

jpountz commented Mar 20, 2018

Superseded by #29165 where I applied @ywelsch's suggestions.

@jpountz jpountz closed this Mar 20, 2018
jpountz added a commit that referenced this pull request Mar 21, 2018
This will reject mapping updates to the `_default_` mapping with 7.x indices
and still emit a deprecation warning with 6.x indices.

Relates #15613
Supersedes #28248
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants