Make PreBuiltAnalyzerProviderFactory plugable via AnalysisPlugin and #31095
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
martijnvg
added
>non-issue
review
:Search Relevance/Analysis
|
Pinging @elastic/es-search-aggs |
75 tasks
nik9000
requested changes
Jun 5, 2018
| Map<String, PreConfiguredTokenFilter> preConfiguredTokenFilters, | ||
| Map<String, PreConfiguredTokenizer> preConfiguredTokenizers) { | ||
| Map<String, PreBuiltAnalyzerProviderFactory> analyzerProviderFactories = new HashMap<>(); | ||
| Map<String, PreConfiguredCharFilter> preConfiguredCharFilters, |
There was a problem hiding this comment.
I think the old indentation was better here.
| return new PreBuiltAnalyzerProvider(name, AnalyzerScope.INDICES, analyzer); | ||
| } | ||
| public PreBuiltAnalyzerProviderFactory(String name, Function<Version, Analyzer> create) { | ||
| this(name, PreBuiltCacheFactory.CachingStrategy.LUCENE, create); |
There was a problem hiding this comment.
I think it'd be nice to remove this second ctor so we're explicit every time.
| } | ||
|
|
||
| public Analyzer analyzer() { | ||
| return analyzerProvider.get(); | ||
| private static class PreBuiltAnalyzersDelegateCache implements PreBuiltCacheFactory.PreBuiltCache<AnalyzerProvider<?>> { |
There was a problem hiding this comment.
I think this is worth some javadoc and comments to explain why we do it. It makes sense but things like an empty put method will confuse future me when I read this class in six months.
There was a problem hiding this comment.
It'd be super nice to explain that we only need this because of PreBuiltAnalyzers.
| throws IOException { | ||
| return create(name, settings); | ||
| public void close() throws IOException { | ||
| IOUtils.close((Closeable) cache.values().stream().map(AnalyzerProvider::get)); |
There was a problem hiding this comment.
It looks to me like this will hit a NullPointerException.
There was a problem hiding this comment.
When you use it with an instance of PreBuiltAnalyzers.
There was a problem hiding this comment.
good catch. I will fix that.
| @@ -162,6 +161,16 @@ public AnalysisRegistry getAnalysisRegistry() { | |||
| return tokenFilters; | |||
| } | |||
|
|
|||
| static Map<String, PreBuiltAnalyzerProviderFactory> setPreBuiltAnalyzerProviderFactory(List<AnalysisPlugin> plugins) { | |||
There was a problem hiding this comment.
setupPreBuiltAnalyzerProviderFactories?
|
Thanks @nik9000 for reviewing. I've updated this PR. |
nik9000
approved these changes
Jun 5, 2018
move `finger_print`, `pattern` and `standard_html_strip` analyzers to analysis-common module. (both AnalysisProvider and PreBuiltAnalyzerProvider) Changed PreBuiltAnalyzerProviderFactory to extend from PreConfiguredAnalysisComponent and changed to make sure that predefined analyzers are always instantiated with the current ES version and if an instance is requested for a different version then delegate to PreBuiltCache. This is similar to the behaviour that exists today in AnalysisRegistry.PreBuiltAnalysis and PreBuiltAnalyzerProviderFactory. Relates to elastic#23658
martijnvg
force-pushed
the
make_PreBuiltAnalyzerProviderFactory_pluggable
branch
from
506bdcc
to
97f7539
Compare
martijnvg
added a commit
that referenced
this pull request
Jun 6, 2018
move `finger_print`, `pattern` and `standard_html_strip` analyzers to analysis-common module. (both AnalysisProvider and PreBuiltAnalyzerProvider) Changed PreBuiltAnalyzerProviderFactory to extend from PreConfiguredAnalysisComponent and changed to make sure that predefined analyzers are always instantiated with the current ES version and if an instance is requested for a different version then delegate to PreBuiltCache. This is similar to the behaviour that exists today in AnalysisRegistry.PreBuiltAnalysis and PreBuiltAnalyzerProviderFactory. (#31095) Relates to #23658
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
move
finger_print,patternandstandard_html_stripanalyzersto analysis-common module. (both AnalysisProvider and PreBuiltAnalyzerProvider)
Changed PreBuiltAnalyzerProviderFactory to extend from PreConfiguredAnalysisComponent and
changed to make sure that predefined analyzers are always instantiated with the current
ES version and if an instance is requested for a different version then delegate to PreBuiltCache.
This is similar to the behaviour that exists today in AnalysisRegistry.PreBuiltAnalysis and
PreBuiltAnalyzerProviderFactory.
Relates to #23658