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 type level default analyzers #9430

Merged
merged 1 commit into from Jan 27, 2015
Merged

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Jan 27, 2015

Notes for reviewers:

  • I moved the Analyzer used by indexing operations in Engine to a separate parameter. The fact that these operations need the DocumentMapper is sneaky, and only used for updating mappings on master. Hopefully future cleanups with how mappings are applied will allow removal of the DocumentMapper parameter altogether. (e.g. Mappings: make mappings immutable #9365).
  • The addition of these settings to static bwc index creation is partial here (only for 1.4.2). There is more work to do on those tests, so I chose to only update one index in order to have some test coverage, but leave full regeneration of all the indexes for a separate issue.

closes #8874

@clintongormley clintongormley added review :Search/Mapping Index mappings, including merging and defining field types labels Jan 27, 2015
@jpountz
Copy link
Contributor

jpountz commented Jan 27, 2015

The change looks good. Something that I'm missing is what the expected behaviour is when a user upgrades from 1.x to 2.x. I see the parsing logic has been removed from DocumentMapperParser so I believe that starting a 1.x index with per-type default analyzers would fail on 2.x?

@clintongormley
Copy link

@jpountz good catch. There are two conflicting needs here:

  • to inform the user that the _analyzer field no longer works
  • to allow the user to continue using their data without reindexing, even though the _analyzer field no longer has any effect

I think we should throw an exception by default, so that the user knows that there is a problem (rather than just silently changing behaviour), but we need to provide some way for the user to say "go ahead and use this index anyway".

Possibilities:

  • apply a setting to the closed index to say "ignore the _analyzer field", then allow opening the index
  • perhaps just upgrade the mapping to remove the _analyzer field if the user manually opens the index (or add a "force" flag to the open API?)
  • add functionality to the upgrade API to make the required mapping changes

any preferences or better ideas?

@rjernst
Copy link
Member Author

rjernst commented Jan 27, 2015

I see the parsing logic has been removed from DocumentMapperParser so I believe that starting a 1.x index with per-type default analyzers would fail on 2.x?

That's what I thought at first, and had extra logic in the parser to log and skip when it saw these old settings. However, I found that this worked even without my extra logic (with the restore bwc tests). The validation function, which checks if there are any extra settings, only throws exception on index creation on or after 2.0. If the index was created before 2.0, it only logs a warning.

@clintongormley This PR is for removing per type analyzer defaults... _analyzer was a different issue, but you are right we should have an issue to address bwc (and I think we can test it similar to what I did here with the static bwc indexes).

@clintongormley
Copy link

@rjernst sorry, mixed up the issues, although the same problems exist here. Let's deal with this problem separately in #9443

@jpountz
Copy link
Contributor

jpountz commented Jan 27, 2015

The validation function, which checks if there are any extra settings, only throws exception on index creation on or after 2.0. If the index was created before 2.0, it only logs a warning.

I had missed it was only enabled for new indices! Thanks for the explanation.

@jpountz
Copy link
Contributor

jpountz commented Jan 27, 2015

LGTM

@rjernst rjernst merged commit cff0ec3 into elastic:master Jan 27, 2015
@rjernst rjernst removed the review label Jan 27, 2015
@rjernst rjernst deleted the fix/8874 branch March 24, 2015 02:56
rjernst added a commit that referenced this pull request Apr 30, 2015
rjernst added a commit that referenced this pull request Apr 30, 2015
@rjernst
Copy link
Member Author

rjernst commented Apr 30, 2015

@clintongormley I removed the documentation for it in master and added a deprecation warning in 1.x.

@clintongormley clintongormley changed the title Mappings: Remove type level default analyzers Remove type level default analyzers Jun 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :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.

Remove type-level analyzer, index_analyzer, search_analyzer
3 participants