Skip to content

Allow reloading of search time analyzers #42888

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

Merged

Conversation

cbuescher
Copy link
Member

Currently changing resources (like dictionaries, synonym files etc...) of search
time analyzers is only possible by closing an index, changing the underlying
resource (e.g. synonym files) and then re-opening the index for the change to
take effect.

This PR adds a new API endpoint that allows triggering reloading of certain
analysis resources (currently token filters) that will then pick up changes in
underlying file resources. To achieve this we introduce a new type of custom
analyzer (ReloadableCustomAnalyzer) that uses a ReuseStrategy that allows
swapping out analysis components. Custom analyzers that contain filters that are
markes as "updateable" will automatically choose this implementation. This PR
also adds this capability to synonym token filters for use in search time
analyzers.

Relates to #29051

Christoph Büscher added 2 commits June 5, 2019 13:26
Currently changing resources (like dictionaries, synonym files etc...) of search
time analyzers is only possible by closing an index, changing the underlying
resource (e.g. synonym files) and then re-opening the index for the change to
take effect.

This PR adds a new API endpoint that allows triggering reloading of certain
analysis resources (currently token filters) that will then pick up changes in
underlying file resources. To achieve this we introduce a new type of custom
analyzer (ReloadableCustomAnalyzer) that uses a ReuseStrategy that allows
swapping out analysis components. Custom analyzers that contain filters that are
markes as "updateable" will automatically choose this implementation. This PR
also adds this capability to `synonym` token filters for use in search time
analyzers.

Relates to elastic#29051
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@cbuescher
Copy link
Member Author

@jimczi @romseygeek I added a commit to make both CustomAnalyzer and ReloadableCustomanalyzer implement a common interface that allows getting the AnalyzerComponents in a uniform way. I also added a unit test for ReloadableCustomanalyzer that tests that changes made via reloading become visible to several concurrently running threads. Let me know what you think about those changes and if they address your previous comments.

@jimczi
Copy link
Contributor

jimczi commented Jun 7, 2019

Let me know what you think about those changes and if they address your previous comments.

+1, the change looks good to me. Any idea why the tests are failing ?

@cbuescher
Copy link
Member Author

@elasticmachine run elasticsearch-ci/1

@cbuescher
Copy link
Member Author

@jimczi tests are green now, I had to undo a change I did in IndexAnalyzers because it interfered with the (somewhat weird) way we use temporary
IndexAnalyzers in `MetaDataIndexUpgradeService#checkMappingsCompatibility) and I intend to try improve on that in another issue but this shouldn't
concern this PR. Anything else to add besides getting anohter thumbs up from @romseygeek and/or @jpountz?

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.

I left some minor comments but the change looks good to me. I am also curious to know what @jpountz and @romseygeek think.

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

One potential simplification and one nit, LGTM otherwise.

@jpountz
Copy link
Contributor

jpountz commented Jun 14, 2019

I'm a bit under water with various stuff, please don't wait for me to merge it.

@cbuescher cbuescher merged commit caae169 into elastic:reloadable-analyzers-feature Jun 14, 2019
@cbuescher
Copy link
Member Author

@jpountz sure I'll ask for your review before finally merging the feature branch to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants