Allow reloading of search time analyzers #42669

cbuescher · 2019-05-29T13:08:03Z

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

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 · 2019-05-29T13:08:52Z

Pinging @elastic/es-search

romseygeek

Looks good! I have a couple of comments, but I like how this is working overall.

server/src/main/java/org/elasticsearch/index/analysis/IndexAnalyzers.java

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestReloadAnalyzersAction.java

romseygeek · 2019-06-04T08:36:39Z

server/src/main/java/org/elasticsearch/index/analysis/ReloadableCustomAnalyzer.java

+        }
+        return reader;
+    }
+}


Given how much of this is shared with CustomAnalyzer, I wonder if we ought to make CustomAnalyzer take AnalyzerComponents as well and then have ReloadableCustomAnalyzer extend it?

That was my first solution in #40782 which Jim was somewhat against. One reason was that this changed CustomAnalyzer to non-final, also it changed the CustomAnalyzers API a bit which we said we'd try to avoid. Happy to go the other way again but would like to get some resolution before reworking everything again here.

I am not worried about the sharing of code, the way we use the components in the reloadable analyzer is quite different and would be difficult to generalize without changing the custom analyzer. However I wonder if we could have a base class similar to TokenFilterComposite but that would give access to the AnalyzerComponents entirely:

public class AbstractCustomAnalyzer { public abstract AnalyzerComponents[] components(); }

This would also resolve my previous comment regarding the way we expose individual component of the AnalyzerComponents in the reloadable analyzer.

I addressed this in eae870f, but decided to go with an interface instead of an abstract class

server/src/main/java/org/elasticsearch/index/analysis/CustomAnalyzerProvider.java

romseygeek · 2019-06-04T08:43:59Z

...es/analysis-common/src/test/java/org/elasticsearch/analysis/common/SynonymAnalyzerTests.java

+    protected Collection<Class<? extends Plugin>> getPlugins() {
+        return Arrays.asList(CommonAnalysisPlugin.class);
+    }
+


Can we add a thread-safety test here as well? ie, create TokenStreams in multiple threads, trigger a reload in a separate thread, and check that no tokenstream ends up with 'mixed' synonyms

I was thinking about that but would like to hear other peoples opinions about what a test like that would really need to be useful. Its a bit of overhead and these concurrency tests are typically a bit flaky, so I only would like to add one if the goals are clear.
Currently what I don't understand about your ask is what "mixed" synonyms would mean. The SynonymMap in SynonymTokenFilterFactory should always be changed as a whole I think, are you saying the we need tests that check that this happens at a specific point in time after another thread triggers the reload? Or do you just want to check that each running thread eventually sees the change?

I think we need a unit test for the ReloadableCustomAnalyzer, something that checks that reloading works and that multiple threads will see the update when they create a new token stream. It doesn't need to be focus on synonyms, just checking that the new filters are taken into account when there is an update should be enough.

cbuescher · 2019-06-04T12:50:11Z

@romseygeek thanks for the review, I addressed your smaller comments and hopefully answered another question. About the additional tests you asked for: happy to add them but would like to get a better picture about what everybody thinks they should be doing before starting on it.

jimczi

I left some comments but it's getting close!

jimczi · 2019-06-05T06:54:31Z

...es/analysis-common/src/test/java/org/elasticsearch/analysis/common/SynonymAnalyzerTests.java

+    protected Collection<Class<? extends Plugin>> getPlugins() {
+        return Arrays.asList(CommonAnalysisPlugin.class);
+    }
+


I think we need a unit test for the ReloadableCustomAnalyzer, something that checks that reloading works and that multiple threads will see the update when they create a new token stream. It doesn't need to be focus on synonyms, just checking that the new filters are taken into account when there is an update should be enough.

jimczi · 2019-06-05T07:01:57Z

server/src/main/java/org/elasticsearch/index/analysis/ReloadableCustomAnalyzer.java

+        }
+        return reader;
+    }
+}


I am not worried about the sharing of code, the way we use the components in the reloadable analyzer is quite different and would be difficult to generalize without changing the custom analyzer. However I wonder if we could have a base class similar to TokenFilterComposite but that would give access to the AnalyzerComponents entirely:

public class AbstractCustomAnalyzer { public abstract AnalyzerComponents[] components(); }

This would also resolve my previous comment regarding the way we expose individual component of the AnalyzerComponents in the reloadable analyzer.

cbuescher · 2019-06-05T11:08:04Z

Sorry, messed up the feature branch, recreating a different one. I might need to open a new PR since I don't see a way to re-open this one.

cbuescher · 2019-06-05T11:31:09Z

I re-created the state of this PR in #42888. Sorry for the noise.

jimczi · 2019-06-05T11:49:27Z

We already lost the comments and reviews of the original pr, now we're loosing the first round of reviews with some open discussions. Isn't it possible to restore this remote branch locally in your checkout ?

cbuescher · 2019-06-05T12:31:24Z

Really sorry and I get the frustration, but I tried it for some time. I accidentally pushed something to the main feature branch this was opened against (naming conflicts being the reason for this accident) so i had to delete that one. Github seems to disallow re-opening sth where the main branch is gone. But I'm going to move the review comments over to the new PR.

jimczi · 2019-06-05T12:56:19Z

Ok, thanks for copying the review comments.

cbuescher added :Search Relevance/Analysis How text is split into tokens >enhancement v7.3.0 v8.0.0 labels May 29, 2019

cbuescher mentioned this pull request May 29, 2019

Enable reloading of search time analyzers #40782

Closed

cbuescher requested review from romseygeek and jpountz June 3, 2019 15:38

romseygeek suggested changes Jun 4, 2019

View reviewed changes

cbuescher force-pushed the reloadable-analyzers branch from 1b440f6 to ed00658 Compare June 4, 2019 12:51

Addressing review comments

44211d8

cbuescher force-pushed the reloadable-analyzers branch from ed00658 to 44211d8 Compare June 4, 2019 14:55

cbuescher mentioned this pull request Jun 4, 2019

Add documentation about reloading search analyzers #42865

Closed

jimczi reviewed Jun 5, 2019

View reviewed changes

cbuescher merged commit 44211d8 into elastic:reloadable-analyzers Jun 5, 2019

cbuescher deleted the reloadable-analyzers branch June 5, 2019 11:16

cbuescher mentioned this pull request Jun 5, 2019

Allow reloading of search time analyzers #42888

Merged

cbuescher mentioned this pull request Jun 5, 2019

Add documentation about reloading search analyzers #42891

Merged

Mpdreamz mentioned this pull request Aug 7, 2019

[meta] 7.3 Release elastic/elasticsearch-net#4001

Closed

16 tasks

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow reloading of search time analyzers #42669

Allow reloading of search time analyzers #42669

cbuescher commented May 29, 2019

elasticmachine commented May 29, 2019

romseygeek left a comment

romseygeek Jun 4, 2019

cbuescher Jun 4, 2019

jimczi Jun 5, 2019

cbuescher Jun 5, 2019

romseygeek Jun 4, 2019

cbuescher Jun 4, 2019

jimczi Jun 5, 2019

cbuescher commented Jun 4, 2019

jimczi left a comment

jimczi Jun 5, 2019

jimczi Jun 5, 2019

cbuescher commented Jun 5, 2019

cbuescher commented Jun 5, 2019

jimczi commented Jun 5, 2019

cbuescher commented Jun 5, 2019

jimczi commented Jun 5, 2019

Allow reloading of search time analyzers #42669

Allow reloading of search time analyzers #42669

Conversation

cbuescher commented May 29, 2019

elasticmachine commented May 29, 2019

romseygeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Jun 4, 2019

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Jun 5, 2019

cbuescher commented Jun 5, 2019

jimczi commented Jun 5, 2019

cbuescher commented Jun 5, 2019

jimczi commented Jun 5, 2019