-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[Analysis] Deprecate standard html analyzer in 6.x #37292
[Analysis] Deprecate standard html analyzer in 6.x #37292
Conversation
Pinging @elastic/es-search |
b4a9850
to
76f6f09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one question, other than that LGTM - no need for another round.
@@ -136,6 +136,11 @@ public Analyzer getAnalyzer(String analyzer) throws IOException { | |||
throw new ElasticsearchException("failed to load analyzer for name " + key, ex); | |||
}} | |||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this part? I think just having the deprecation message in the AnalyzerProvider should be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this part for this line.
Line 326 in 76f6f09
analyzers.add(new PreBuiltAnalyzerProviderFactory("standard_html_strip", CachingStrategy.ELASTICSEARCH, |
If Es find
standard_html_strip
from prebuiltAnalysis
, Es doesn't use StandardHtmlStripAnalyzerProvider
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok. Can you move the deprecation message into the PreBuiltAnalyzerProviderFactory? Like
Line 440 in 76f6f09
filters.add(PreConfiguredTokenFilter.singletonWithVersion("nGram", false, (reader, version) -> { |
I'd rather not have specific analyzers referred to in the AnalysisRegistry if we can avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I pushed it but it failed.
I think the timing to call create
function is different between PreBuiltAnalyzerProviderFactory
and PreConfiguredTokenFilter
.
And I just remembered why I added check logic in AnalysisRegistry...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romseygeek Thanks! I will revert last commit, then will merge it.
a86619a
to
76f6f09
Compare
@elasticmachine test this please |
1 similar comment
@elasticmachine test this please |
76f6f09
to
e942d63
Compare
Backport elastic#26719 to 6.x Closes elastic#4704 (cherry picked from commit 38b698d)
e942d63
to
9828dd0
Compare
Deprecate standard_html_strip analyzer in 6.x
Related #4704