Check for deprecations when analyzers are built #50908

romseygeek · 2020-01-13T11:32:54Z

Generally speaking, deprecated analysis components in elasticsearch will issue deprecation
warnings when they are first used. However, this means that no warnings are emitted when
indexes are created with deprecated components, and users have to actually index a document
to see warnings. This makes it much harder to see these warnings and act on them at
appropriate times.

This commit adds a new check that pushes an empty string through all user-defined analyzers
and normalizers when an IndexAnalyzers object is built for each index; deprecation warnings
are now emitted when indexes are created or opened.

Fixes #42349

elasticmachine · 2020-01-13T11:32:57Z

Pinging @elastic/es-search (:Search/Analysis)

cbuescher

@romseygeek I took a first look and was wondering how version-dependent logic like deprecation from V7.6 on and throwing errors starting with a later version of the analysis components would work. Can you add a small example for this maybe in some tests?

cbuescher

I left a short question, rest looks good to me.

cbuescher · 2020-01-13T15:55:14Z

server/src/test/java/org/elasticsearch/action/admin/indices/TransportAnalyzeActionTests.java

+        analyze =
+            TransportAnalyzeAction.analyze(req, registry, mockIndexService(), maxTokenCount);
+        assertEquals(1, analyze.getTokens().size());
+        assertWarnings("Using deprecated token filter [deprecated]");


Just a small observation: I thought this checks that the normalize method in DeprecatedTokenFilterFactory, so I tried changing it but the test didn't fail, do you know why?

Because the default implementation of normalize delegates to create, so we get the warning from there instead.

So that sounds like the "normalize()" method isn't actualy tested here? I saw we cover that in some other test though, so fine with whatever you decide doing here, I was just curious if this could be checked here through the "_analyze" API as well, but no problem it its too tricky I.

No, it's tested, but in order to make it fail you can't just remove the method (because that then delegates to create()), you have to have an implementation that doesn't emit a warning

Oh, I've just realised that we're talking about the AnalyzeAction - this always uses create() rather than normalize(), and we should probably change that.

we should probably change that

This can most likely be done in a follow up PR though, I just wanted to understand whats going on here and which of the two methods in the DeprecatedTokenFilterFactory in this test is supposed to fire the warning, currently they emit the same text so its hard to tell which code path is checked.

jimczi

I left two minor comments that would be nice to address, LGTM otherwise.

jimczi · 2020-01-13T21:05:39Z

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

+
+    // Deprecation warnings are emitted when actual TokenStreams are built; this is usually
+    // too late to be useful, so we build an empty tokenstream at construction time and
+    // use it, to ensure that warnings are emitted immediately.


We also throw exceptions in some cases so it's not only about deprecations ?

jimczi · 2020-01-13T21:06:29Z

server/src/test/java/org/elasticsearch/index/analysis/AnalysisRegistryTests.java

@@ -264,4 +271,90 @@ public void testEnsureCloseInvocationProperlyDelegated() throws IOException {
        registry.close();
        verify(mock).close();
    }
+
+    public void testDeprecations() throws IOException {


Can you also add a test that throws an exception ?

…ns-check

romseygeek · 2020-01-14T13:12:06Z

Generally speaking, deprecated analysis components in elasticsearch will issue deprecation
warnings when they are first used. However, this means that no warnings are emitted when
indexes are created with deprecated components, and users have to actually index a document
to see warnings. This makes it much harder to see these warnings and act on them at
appropriate times.

This is worse in the case where components throw exceptions on upgrade. In this case, users
will not be aware of a problem until a document is indexed, instead of at index creation time.

This commit adds a new check that pushes an empty string through all user-defined analyzers
and normalizers when an IndexAnalyzers object is built for each index; deprecation warnings
and exceptions are now emitted when indexes are created or opened.

Fixes #42349

Generally speaking, deprecated analysis components in elasticsearch will issue deprecation warnings when they are first used. However, this means that no warnings are emitted when indexes are created with deprecated components, and users have to actually index a document to see warnings. This makes it much harder to see these warnings and act on them at appropriate times. This is worse in the case where components throw exceptions on upgrade. In this case, users will not be aware of a problem until a document is indexed, instead of at index creation time. This commit adds a new check that pushes an empty string through all user-defined analyzers and normalizers when an IndexAnalyzers object is built for each index; deprecation warnings and exceptions are now emitted when indexes are created or opened. Fixes #42349

Generally speaking, deprecated analysis components in elasticsearch will issue deprecation warnings when they are first used. However, this means that no warnings are emitted when indexes are created with deprecated components, and users have to actually index a document to see warnings. This makes it much harder to see these warnings and act on them at appropriate times. This is worse in the case where components throw exceptions on upgrade. In this case, users will not be aware of a problem until a document is indexed, instead of at index creation time. This commit adds a new check that pushes an empty string through all user-defined analyzers and normalizers when an IndexAnalyzers object is built for each index; deprecation warnings and exceptions are now emitted when indexes are created or opened. Fixes elastic#42349

The lucene Ukrainian analyzer has a bug where a large in-memory dictionary is loaded and stored on a thread local for every tokenstream generated in a new thread (for more details see https://issues.apache.org/jira/browse/LUCENE-9930). Due to checks added in #50908, we create a tokenstream for every registered analyzer in every shard, which means that any node with the ukrainian plugin installed will leak one copy of this dictionary per shard, whether or not the ukrainian analyzer is actually being used. This commit makes the plugin use a fixed version of the UkrainianMorfologikAnalyzer, until we merge a version of lucene that contains the upstream fix.

Check for deprecations when analyzers are built

54c659c

romseygeek added >enhancement :Search Relevance/Analysis How text is split into tokens v8.0.0 v7.6.0 labels Jan 13, 2020

romseygeek self-assigned this Jan 13, 2020

checkstyle; tests

1ca8b8b

cbuescher reviewed Jan 13, 2020

View reviewed changes

Add version check

bcdd2b0

cbuescher reviewed Jan 13, 2020

View reviewed changes

cbuescher approved these changes Jan 13, 2020

View reviewed changes

jimczi approved these changes Jan 13, 2020

View reviewed changes

cbuescher mentioned this pull request Jan 14, 2020

Deprecate and remove camel-case nGram and edgeNGram tokenizers #50862

Merged

romseygeek added 3 commits January 14, 2020 09:36

Add tests for exceptions as well

4941470

Merge remote-tracking branch 'origin/master' into analysis/deprecatio…

9a18f62

…ns-check

checkstyle

d938eaa

romseygeek merged commit 736ed47 into elastic:master Jan 14, 2020

romseygeek deleted the analysis/deprecations-check branch January 14, 2020 13:12

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

romseygeek mentioned this pull request Apr 21, 2021

Ukrainian language plugin can fill up heap #71998

Merged

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

Check for deprecations when analyzers are built #50908

Check for deprecations when analyzers are built #50908

romseygeek commented Jan 13, 2020

elasticmachine commented Jan 13, 2020

cbuescher left a comment

cbuescher left a comment

cbuescher Jan 13, 2020

romseygeek Jan 13, 2020

cbuescher Jan 13, 2020

romseygeek Jan 13, 2020

romseygeek Jan 13, 2020

cbuescher Jan 13, 2020

jimczi left a comment

jimczi Jan 13, 2020

jimczi Jan 13, 2020

romseygeek commented Jan 14, 2020

Check for deprecations when analyzers are built #50908

Check for deprecations when analyzers are built #50908

Conversation

romseygeek commented Jan 13, 2020

elasticmachine commented Jan 13, 2020

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher 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

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

romseygeek commented Jan 14, 2020