[Analysis] Deprecate Standard Html Strip Analyzer in master #26719

johtani · 2017-09-20T07:52:27Z

Deprecate standard_html_strip analyzer in 6.x

Add deprecation log if using the analyzer
Cannot create index with the analyzer after 6.1.0

I will make PR for removing the analyzer on master

jasontedor · 2017-09-20T22:33:25Z

Cannot create index with the analyzer after 6.1.0

This concerns me as a breaking change that we should not make in a minor release? I think we can only deprecate in 6.x but not break until 7.0.0.

johtani · 2017-09-20T23:52:35Z

@jasontedor You are right. I will change it for onlt deprecation logging.
Can we remove the analyzer in 7.0?

jasontedor · 2017-09-23T02:11:44Z

Can we remove the analyzer in 7.0?

Is it needed in 7.x to support indices created in 6.x with the analyzer? If this is correct, I think we have to wait until 8.0.0 to remove?

romseygeek · 2018-03-14T10:34:09Z

cc @elastic/es-search-aggs

colings86 · 2018-10-24T10:52:27Z

@johtani @romseygeek Is this still something that needs to be done? If so what should we do to get this merged?

romseygeek · 2018-10-24T10:55:04Z

We should still do this, I think. @johtani do you have time to take this up?

johtani · 2018-10-25T04:49:55Z

@colings86 @romseygeek Oh, sorry. Yes, I have. I will update this week.

johtani · 2018-10-28T02:07:42Z

@romseygeek I've updated this. Could you review this?

romseygeek

Thanks @johtani. I left some comments.

romseygeek · 2018-10-28T10:27:27Z

docs/reference/migration/migrate_6_1.asciidoc

@@ -1,8 +1,5 @@
 [[breaking-changes-6.1]]
 == Breaking changes in 6.1
-++++


I think this is left over from a previous version?

Good catch...

romseygeek · 2018-10-28T10:28:31Z

...ommon/src/main/java/org/elasticsearch/analysis/common/StandardHtmlStripAnalyzerProvider.java

+        if (indexSettings.getIndexVersionCreated().onOrAfter(Version.V_6_5_0)) {
+            DEPRECATION_LOGGER.deprecatedAndMaybeLog("standard_html_strip_deprecation",
+                "Deprecated analyzer [standard_html_strip] used, " +
+                    "replaced by using [html_strip] char_filter");


nit: s/replaced/replace/

Should probably also be explicit that you need to replace it with a custom analyzer using standard tokenizer and html_strip char_filter, plus lowercase filter and any other filters you have been using.

romseygeek · 2018-10-28T10:31:48Z

...alysis-common/src/test/java/org/elasticsearch/analysis/common/CommonAnalysisPluginTests.java

+    /**
+     * Check that the deprecated analyzer name "standard_html_strip" issues a deprecation warning for indices created before 6.4.0
+     */
+    public void testStandardHtmlStripAnalyzerNoDeprecationPre6_5() throws IOException {


This test doesn't seem to do what the comment suggests it should do? I think it can be repurposed to check that an exception is thrown if you try to create an analyzer of type standard_html_strip with an index created version greater than or equal to 7.0, and the test above can just check that the warning is emitted on any version before 7.0

Ah, good catch. I forgot add "does NOT" after copy & paste...

I'm not sure your comment about 7.0. Is your comment about PR for master branch?

Ah, I hadn't noticed that this was against 6.x. Do you have a separate PR against master? I think it makes most sense to do this against master and then backport?

Ah, I made this PR for logging deprecation and I thought we removeed this analyzer in 7 at that time.

I agree to do against master first. I will change this PR to against 7. sorry

romseygeek · 2018-10-28T10:32:30Z

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

+        } else if ("standard_html_strip".equals(analyzer)) {
+            DEPRECATION_LOGGER.deprecatedAndMaybeLog("standard_html_strip_deprecation",
+                "Deprecated analyzer [standard_html_strip] used, " +
+                    "replaced by using [html_strip] char_filter");


s/replaced/replace/

romseygeek · 2018-10-28T10:35:21Z

docs/reference/migration/migrate_6_5.asciidoc

+==== Deprecated standard_html_strip analyzer
+
+ Deprecated standard_html_strip analyser in 6.5.
+ This will be not create in 7.0 and be removed in 8.0.


How about:

The standard_html_strip analyzer has been deprecated, and should be replaced with a combination of the standard tokenizer and html_strip char_filter. Indexes created using this analyzer will still be readable in elasticsearch 7.0, but it will not be possible to create new indexes using it.

johtani · 2018-11-02T12:21:42Z

@romseygeek Change this to against master branch. Please review again if you have time :)

romseygeek

I left a couple more comments.

romseygeek · 2018-11-02T14:46:52Z

...alysis-common/src/test/java/org/elasticsearch/analysis/common/CommonAnalysisPluginTests.java

+                "use a custom analyzer using [standard] tokenizer and [html_strip] char_filter, plus [lowercase] filter");
+        } catch (Exception e) {
+            fail("expected IAE");
+        }


Can you use expectThrows here?

...ommon/src/main/java/org/elasticsearch/analysis/common/StandardHtmlStripAnalyzerProvider.java

johtani · 2018-11-05T07:48:47Z

@romseygeek Thanks for you comments. Fix these. Your 2nd comment means that we should issue deprecation log for any index created with 6.x, right?

I have a question about test.

How do we test 6.6.1 <= versions < 7.0.0_alpha1 with VersionUtils.randomVersionBetween(...) ?

romseygeek · 2018-11-06T09:18:33Z

How do we test 6.6.1 <= versions < 7.0.0_alpha1 with VersionUtils.randomVersionBetween(...) ?

You can use VersionUtils.getPreviousVersion(Version.V_7_0_0_alpha1) to get the upper bound, I think?

LGTM otherwise. Thanks!

johtani · 2018-11-08T07:49:42Z

retest this please

pcsanwald · 2019-01-03T21:27:51Z

@elasticmachine test this please

Deprecate only Standard Html Strip Analyzer If user create index with the analyzer since 7.0, es throws an exception. If an index was created before 7.0, es issue deprecation log We will remove it in 8.0 Related elastic#4704

Use expectThrows Change versions for deprecation log Related elastic#4704

Fix checkstyle error Related elastic#4704

Change to use getPreviousVersion Related elastic#4704

Change version to 7_0_0 and fix deprecationlogger error Related elastic#4704

Backport elastic#26719 to 6.x Closes elastic#4704 (cherry picked from commit 38b698d)

Backport #26719 to 6.x Related #4704 (cherry picked from commit 38b698d)

johtani added :Search/Analysis How text is split into tokens v6.1.0 labels Sep 20, 2017

johtani force-pushed the remove_standard_html_analyzer branch 2 times, most recently from 7a49f86 to 1364d3f Compare October 7, 2017 12:19

lcawl added v6.2.0 and removed v6.1.0 labels Dec 12, 2017

colings86 added v6.3.0 and removed v6.2.0 labels Jan 22, 2018

colings86 added the >deprecation label Apr 24, 2018

bleskes added v6.3.1 v6.4.0 and removed v6.3.0 v6.3.1 labels Apr 26, 2018

lcawl added v6.4.1 and removed v6.4.0 labels Aug 23, 2018

johtani force-pushed the remove_standard_html_analyzer branch from 1364d3f to af9ccc5 Compare October 26, 2018 05:44

romseygeek suggested changes Oct 28, 2018

View reviewed changes

johtani force-pushed the remove_standard_html_analyzer branch from d6bf840 to 613c6de Compare November 1, 2018 17:10

johtani changed the base branch from 6.x to master November 1, 2018 17:10

johtani changed the title ~~[Analysis] Deprecate Standard Html Strip Analyzer in 6.x~~ [Analysis] Deprecate Standard Html Strip Analyzer in master Nov 2, 2018

johtani force-pushed the remove_standard_html_analyzer branch 2 times, most recently from fe0ed0c to 76a42c5 Compare November 2, 2018 06:01

romseygeek suggested changes Nov 2, 2018

View reviewed changes

romseygeek approved these changes Nov 6, 2018

View reviewed changes

johtani force-pushed the remove_standard_html_analyzer branch from 2175c32 to b2bbf27 Compare November 14, 2018 06:48

tomcallahan added v7.0.0 and removed v6.4.1 labels Jan 3, 2019

johtani added 4 commits January 8, 2019 11:48

[Analysis] Deprecate Standard Html Strip Analyzer

4a3ab58

Deprecate only Standard Html Strip Analyzer If user create index with the analyzer since 7.0, es throws an exception. If an index was created before 7.0, es issue deprecation log We will remove it in 8.0 Related elastic#4704

[Analysis] Deprecate Standard Html Strip Analyzer

c513987

Use expectThrows Change versions for deprecation log Related elastic#4704

[Analysis] Deprecate Standard Html Strip Analyzer

20402fe

Fix checkstyle error Related elastic#4704

[Analysis] Deprecate Standard Html Strip Analyzer

cbe7243

Change to use getPreviousVersion Related elastic#4704

johtani force-pushed the remove_standard_html_analyzer branch 2 times, most recently from 1531b72 to af6c151 Compare January 8, 2019 06:20

[Analysis] Deprecate Standard Html Strip Analyzer

51bdace

Change version to 7_0_0 and fix deprecationlogger error Related elastic#4704

johtani force-pushed the remove_standard_html_analyzer branch from af6c151 to 51bdace Compare January 8, 2019 07:41

johtani merged commit 38b698d into elastic:master Jan 9, 2019

johtani mentioned this pull request Jan 10, 2019

[Analysis] Deprecate standard html analyzer in 6.x #37292

Merged

johtani added a commit to johtani/elasticsearch that referenced this pull request Jan 11, 2019

[Analysis] Deprecate Standard Html Strip Analyzer in 6.x

b4a9850

Backport elastic#26719 to 6.x Closes elastic#4704 (cherry picked from commit 38b698d)

johtani added a commit to johtani/elasticsearch that referenced this pull request Jan 11, 2019

[Analysis] Deprecate Standard Html Strip Analyzer in 6.x

76f6f09

Backport elastic#26719 to 6.x Closes elastic#4704 (cherry picked from commit 38b698d)

johtani added a commit to johtani/elasticsearch that referenced this pull request Jan 17, 2019

[Analysis] Deprecate Standard Html Strip Analyzer in 6.x

e942d63

Backport elastic#26719 to 6.x Closes elastic#4704 (cherry picked from commit 38b698d)

johtani added a commit to johtani/elasticsearch that referenced this pull request Jan 18, 2019

[Analysis] Deprecate Standard Html Strip Analyzer in 6.x

9828dd0

Backport elastic#26719 to 6.x Closes elastic#4704 (cherry picked from commit 38b698d)

johtani added a commit that referenced this pull request Jan 22, 2019

[Analysis] Deprecate Standard Html Strip Analyzer in 6.x (#37292)

9b51025

Backport #26719 to 6.x Related #4704 (cherry picked from commit 38b698d)

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

cbuescher mentioned this pull request Jun 21, 2019

Remove rests of StandardHtmlStripAnalyzer #43485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Analysis] Deprecate Standard Html Strip Analyzer in master #26719

[Analysis] Deprecate Standard Html Strip Analyzer in master #26719

johtani commented Sep 20, 2017

jasontedor commented Sep 20, 2017

johtani commented Sep 20, 2017

jasontedor commented Sep 23, 2017

romseygeek commented Mar 14, 2018

colings86 commented Oct 24, 2018

romseygeek commented Oct 24, 2018

johtani commented Oct 25, 2018

johtani commented Oct 28, 2018

romseygeek left a comment

romseygeek Oct 28, 2018

johtani Nov 1, 2018

romseygeek Oct 28, 2018

romseygeek Oct 28, 2018

romseygeek Oct 28, 2018

johtani Nov 1, 2018

romseygeek Nov 1, 2018

johtani Nov 1, 2018

romseygeek Oct 28, 2018

romseygeek Oct 28, 2018

johtani commented Nov 2, 2018

romseygeek left a comment

romseygeek Nov 2, 2018

johtani commented Nov 5, 2018

romseygeek commented Nov 6, 2018

johtani commented Nov 8, 2018

pcsanwald commented Jan 3, 2019

[Analysis] Deprecate Standard Html Strip Analyzer in master #26719

[Analysis] Deprecate Standard Html Strip Analyzer in master #26719

Conversation

johtani commented Sep 20, 2017

jasontedor commented Sep 20, 2017

johtani commented Sep 20, 2017

jasontedor commented Sep 23, 2017

romseygeek commented Mar 14, 2018

colings86 commented Oct 24, 2018

romseygeek commented Oct 24, 2018

johtani commented Oct 25, 2018

johtani commented Oct 28, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johtani commented Nov 2, 2018

romseygeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johtani commented Nov 5, 2018

romseygeek commented Nov 6, 2018

johtani commented Nov 8, 2018

pcsanwald commented Jan 3, 2019