Some internal refactorings in AnalysisRegistry #40609
Conversation
cbuescher
added
>non-issue
:Search Relevance/Analysis
|
Pinging @elastic/es-search |
Reducing some methods scope and marking them as static where possible. Removing "alias" support from AnalysisRegistry#produceAnalyze and changing that method to return a NamedAnalyzer instead of having a side effect on the analyzer map passed in. Also, CustomAnalyzerProvider doesn't seem to need the environment field.
cbuescher
force-pushed
the
small-Refactoring-AnalysisRegistry
branch
from
5c306c4 to
86adf42
Compare
| charFilterFactoryFactories, tokenizerFactoryFactories), (k, v) -> { | ||
| throw new IllegalStateException("already registered analyzer with name: " + entry.getKey()); | ||
| }); | ||
| final String analyzerAliasKey = "index.analysis.analyzer." + entry.getKey() + ".alias"; |
There was a problem hiding this comment.
The original code had this comment: // TODO: remove alias support completely when we no longer support pre 5.0 indices
Should we remove this part as we don't support pre 5.0 indexes any more?
There was a problem hiding this comment.
Thanks, I was wondering the same. I understood the comment to mean we want to remove the alias support (which I think we have already) but not the error that prevents these settings to be silently ignored going forward. But I will take another look and maybe consult with sb. else before proceeding to merge this.
There was a problem hiding this comment.
+1 to remove, this is not supported nor documented since version 5.
There was a problem hiding this comment.
@cbuescher Thanks, very nice refactoring indeed. Side effects are difficult to track, and returning a value instead is much better. I wonder if we should do the same about processNormalizerFactory.
Good point, I now saw they look quite similar, I didn't touch that method in my own refactorings so I missed it. I'd like to do that in a follow up though if possible. |
Reducing some methods scope and marking them as static where possible. Removing "alias" support from AnalysisRegistry#produceAnalyze and changing that method to return a NamedAnalyzer instead of having a side effect on the analyzer map passed in. Also, CustomAnalyzerProvider doesn't seem to need the `environment` field.
Reducing some methods scope and marking them as static where possible. Removing "alias" support from AnalysisRegistry#produceAnalyze and changing that method to return a NamedAnalyzer instead of having a side effect on the analyzer map passed in. Also, CustomAnalyzerProvider doesn't seem to need the `environment` field.
5 participants
This PR reduces some method scopes and makes a few methods static where they don't use instance fields.
Also slightly changing AnalysisRegistry#produceAnalyze return a NamedAnalyzer instead of having
a side effect on the analyzer map passed in. In addition, removing the unused
environmentfield from CustomAnalyzerProvider.