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.

Sign up for GitHub

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

Jump to bottom

Check presence of multi-types before validating new mapping #29316

Merged
merged 4 commits into from
Apr 4, 2018
Merged

Check presence of multi-types before validating new mapping #29316

merged 4 commits into from
Apr 4, 2018

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Mar 30, 2018

Before doing any kind of validation on a new mapping, we should first do the multi-type validation in order to provide better error messages. For #29313, this means that the exception message will be
Rejecting mapping update to [range_index_new] as the final mapping would have more than 1 type: [_doc, mytype]
instead of
[expected_attendees] is defined as an object in mapping [mytype] but this name is already used for a field in other types"

@ywelsch ywelsch added >enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0 v6.3.0 labels Mar 30, 2018
@ywelsch ywelsch requested a review from jpountz March 30, 2018 12:29
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jkakavas jkakavas mentioned this pull request Mar 30, 2018
Closed
jpountz
jpountz approved these changes Apr 4, 2018
View reviewed changes
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Thanks @ywelsch.

@ywelsch ywelsch merged commit 1891d4f into elastic:master Apr 4, 2018
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 4, 2018
@jasontedor
Merge branch 'master' into byte-size-value-setting-limit-error-message
09a78ac 
* master:
  Add awaits fix for a query analyzer test
  Check presence of multi-types before validating new mapping (elastic#29316)
ywelsch
ywelsch commented Apr 4, 2018
View reviewed changes
server/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java
/**
* Asserts that the root cause of mapping conflicts is readable.
*/
public void testMappingConflictRootCause() throws Exception {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

while backporting to 6.x, I've reenabled this test (but set index.version.created to 5.x)

ywelsch added a commit that referenced this pull request Apr 4, 2018
@ywelsch
Check presence of multi-types before validating new mapping (#29316)
e97c88f 
Before doing any kind of validation on a new mapping, we should first do the multi-type validation in
order to provide better error messages. For #29313, this means that the exception message will be
Rejecting mapping update to [range_index_new] as the final mapping would have more than 1 type:
[_doc, mytype]
instead of
[expected_attendees] is defined as an object in mapping [mytype] but this name is already used for
a field in other types
martijnvg added a commit that referenced this pull request Apr 5, 2018
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
1f306c3 
* es/master: (68 commits)
  Allow using distance measure in the geo context precision (#29273)
  Disable failing query in QueryBuilderBWCIT.
  Fixed quote_field_suffix in query_string (#29332)
  Use fixture to test repository-url module (#29355)
  Remove undocumented action.master.force_local setting (#29351)
  Enhance error for out of bounds byte size settings (#29338)
  Fix QueryAnalyzerTests.
  Fix HasChildQueryBuilderTests to not use the `classic` similarity.
  [Docs] Correct javadoc of GetIndexRequest (#29364)
  Make TransportRankEvalAction members final
  Add awaits fix for a query analyzer test
  Check presence of multi-types before validating new mapping (#29316)
  Add awaits fix for HasChildQueryBuilderTests
  Remove silent batch mode from install plugin (#29359)
  Align cat thread pool info to thread pool config (#29195)
  Track Lucene operations in engine explicitly (#29357)
  Build: Fix Java9 MR build (#29312)
  Reindex: Fix error in delete-by-query rest spec (#29318)
  Improve similarity integration. (#29187)
  Fix some query extraction bugs. (#29283)
  ...
martijnvg added a commit that referenced this pull request Apr 5, 2018
@martijnvg
Merge remote-tracking branch 'es/6.x' into ccr-6.x
c86e664 
* es/6.x: (68 commits)
  Add note to migration docs on silent batch mode (#29365)
  Allow using distance measure in the geo context precision (#29273)
  Disable failing query in QueryBuilderBWCIT.
  Improve similarity integration. (#29187)
  Fix some query extraction bugs. (#29283)
  Fixed quote_field_suffix in query_string (#29332)
  TEST: Update negative byte size setting error msg
  Fix bwc in GeoDistanceQuery serialization (#29325)
  Move testMappingConflictRootCause to different class
  Enhance error for out of bounds byte size settings (#29338)
  [Docs] Correct javadoc of GetIndexRequest (#29364)
  Check presence of multi-types before validating new mapping (#29316)
  Make TransportRankEvalAction members final
  Pass through script params in scripted metric agg (#29154)
  Remove silent batch mode from install plugin (#29359)
  Track Lucene operations in engine explicitly (#29357)
  Build: Fix Java9 MR build (#29312)
  Reindex: Fix error in delete-by-query rest spec (#29318)
  Move Nullable into core (#29341)
  [Docs] Correct experimental note formatting
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 5, 2018
@jasontedor
Merge branch 'master' into thread-pool-info-migration
c7dae3c 
* master: (110 commits)
  Remove undocumented action.master.force_local setting (elastic#29351)
  Enhance error for out of bounds byte size settings (elastic#29338)
  Fix QueryAnalyzerTests.
  Fix HasChildQueryBuilderTests to not use the `classic` similarity.
  [Docs] Correct javadoc of GetIndexRequest (elastic#29364)
  Make TransportRankEvalAction members final
  Add awaits fix for a query analyzer test
  Check presence of multi-types before validating new mapping (elastic#29316)
  Add awaits fix for HasChildQueryBuilderTests
  Remove silent batch mode from install plugin (elastic#29359)
  Align cat thread pool info to thread pool config (elastic#29195)
  Track Lucene operations in engine explicitly (elastic#29357)
  Build: Fix Java9 MR build (elastic#29312)
  Reindex: Fix error in delete-by-query rest spec (elastic#29318)
  Improve similarity integration. (elastic#29187)
  Fix some query extraction bugs. (elastic#29283)
  [Docs] Correct experimental note formatting
  Move Nullable into core (elastic#29341)
  [Docs] Update getting-started.asciidoc (elastic#29294)
  Elasticsearch 6.3.0 is now on Lucene 7.3.
  ...
@dpursehouse
Copy link
Contributor

Queries that were working against 6.3.2 are now failing against 6.4.0 with the message ...as the final mapping would have more than 1 type..., but neither this PR (#29316) nor the one it references (#29313) are mentioned as breaking changes in the release notes.

@dpursehouse
Copy link
Contributor

Sorry, disregard my previous comment. It was my mistake.

@jtibshirani jtibshirani mentioned this pull request Aug 2, 2019
Merged
jtibshirani added a commit that referenced this pull request Aug 12, 2019
@jtibshirani
Make sure to validate the type before attempting to merge a new mappi…
abb30f0 
…ng. (#45157)

Currently, when adding a new mapping, we attempt to parse + merge it before
checking whether its top-level document type matches the existing type. So when
a user attempts to introduce a new mapping type, we may give a confusing error
message around merging instead of complaining that it's not possible to add
more than one type ("Rejecting mapping update to [my-index] as the final
mapping would have more than 1 type...").

This PR moves the type validation to the start of
`MetaDataMappingService#applyRequest` so that we make sure the type matches
before performing any mapper merging.

We already partially addressed this issue in #29316, but the tests there
focused on `MapperService` and did not catch this problem with end-to-end
mapping updates.

Addresses #43012.
jtibshirani added a commit that referenced this pull request Aug 12, 2019
@jtibshirani
Make sure to validate the type before attempting to merge a new mappi…
dc1856c 
…ng. (#45157)

Currently, when adding a new mapping, we attempt to parse + merge it before
checking whether its top-level document type matches the existing type. So when
a user attempts to introduce a new mapping type, we may give a confusing error
message around merging instead of complaining that it's not possible to add
more than one type ("Rejecting mapping update to [my-index] as the final
mapping would have more than 1 type...").

This PR moves the type validation to the start of
`MetaDataMappingService#applyRequest` so that we make sure the type matches
before performing any mapper merging.

We already partially addressed this issue in #29316, but the tests there
focused on `MapperService` and did not catch this problem with end-to-end
mapping updates.

Addresses #43012.
jtibshirani added a commit that referenced this pull request Aug 12, 2019
@jtibshirani
Make sure to validate the type before attempting to merge a new mappi…
6db9df9 
…ng. (#45157)

Currently, when adding a new mapping, we attempt to parse + merge it before
checking whether its top-level document type matches the existing type. So when
a user attempts to introduce a new mapping type, we may give a confusing error
message around merging instead of complaining that it's not possible to add
more than one type ("Rejecting mapping update to [my-index] as the final
mapping would have more than 1 type...").

This PR moves the type validation to the start of
`MetaDataMappingService#applyRequest` so that we make sure the type matches
before performing any mapper merging.

We already partially addressed this issue in #29316, but the tests there
focused on `MapperService` and did not catch this problem with end-to-end
mapping updates.

Addresses #43012.
@ebuildy
Copy link
Contributor

ebuildy commented Apr 16, 2020

Why this is loggued with DEBUG level ?

[2020-04-16T12:40:57,790][DEBUG][o.e.a.b.TransportShardBulkAction] [events-2020.04.16][0] failed to execute bulk item (index) BulkShardRequest [[events-2020.04.16][0]] containing [index {[events-2020.04.16][doc][uWoBg3EBC9-1w63Yrl-y], source....]
java.lang.IllegalArgumentException: Rejecting mapping update to [events-2020.04.16] as the final mapping would have more than 1 type: [_doc, doc]
	at org.elasticsearch.index.mapper.MapperService.internalMerge(MapperService.java:408) ~[elasticsearch-6.3.2.jar:6.3.2]
	at org.elasticsearch.index.mapper.MapperService.internalMerge(MapperService.java:356) ~[elasticsearch-6.3.2.jar:6.3.2]
	at org.elasticsearch.index.mapper.MapperService.merge(MapperService.java:288) ~[elasticsearch-6.3.2.jar:6.3.2]

This should be an ERROR isnit?

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

@jpountz jpountz jpountz approved these changes

Assignees
No one assigned
Labels
>enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types v6.3.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ywelsch @elasticmachine @dpursehouse @ebuildy @jpountz @colings86