Auto-reload analyzers for specific resource #96986
Conversation
mayya-sharipova
added
>non-issue
:Search Relevance/Analysis
|
Pinging @elastic/es-search (Team:Search) |
mayya-sharipova
force-pushed
the
reload-analyzers-with-resource
branch
from
3fbe5df to
bc2d21b
Compare
...alysis-common/src/main/java/org/elasticsearch/analysis/common/SynonymTokenFilterFactory.java
Outdated
Show resolved
Hide resolved
| */ | ||
| public ReloadAnalyzersRequest(String... indices) { | ||
| public ReloadAnalyzersRequest(String resource, String... indices) { |
There was a problem hiding this comment.
Should we add another constructor that does not have resource as parameter, to avoid passing null?
There was a problem hiding this comment.
I explored this, but this is not possible as indices is varargs of the same type of String.
Making two constructors, and doing ReloadAnalyzersRequest(resource1, index1) throws an error:
reference to ReloadAnalyzersRequest is ambiguous
| both constructor ReloadAnalyzersRequest(java.lang.String...).. and constructor ReloadAnalyzersRequest(java.lang.String,java.lang.String...) ... match
...s-common/src/main/java/org/elasticsearch/analysis/common/SynonymGraphTokenFilterFactory.java
Show resolved
Hide resolved
| match: | ||
| my_field: | ||
| query: salute | ||
| - match: { hits.total.value: 1 } |
There was a problem hiding this comment.
Great test! 💯
Is it too redundant to query the other index to ensure nothing changed?
There was a problem hiding this comment.
Addressed in de85b76. But indeed looks redundant as we did not change the synonyms set that other index is based on.
server/src/main/java/org/elasticsearch/action/admin/indices/analyze/ReloadAnalyzersRequest.java
Show resolved
Hide resolved
| @@ -95,7 +95,7 @@ default NamedAnalyzer getDefaultSearchQuoteAnalyzer() { | |||
| /** | |||
| * Reload any analyzers that have reloadable components | |||
| */ | |||
| default List<String> reload(AnalysisRegistry analysisRegistry, IndexSettings indexSettings) throws IOException { | |||
| default List<String> reload(AnalysisRegistry analysisRegistry, IndexSettings indexSettings, String resource) throws IOException { | |||
There was a problem hiding this comment.
Should we add another method that has no resource on it, to avoid invoking this with explicit nulls?
There was a problem hiding this comment.
We never call it with explicit "null", so having a single constructor is enough.
mayya-sharipova
force-pushed
the
reload-analyzers-with-resource
branch
from
bc2d21b to
f68df16
Compare
|
@elasticsearchmachine run elasticsearch-ci/part-1 |
|
Thanks for the changes, Mayya! LGTM 👍 |
This PR adds a new optional parameter "resource" for ReloadAnalyzersRequest. If used, only analyzers that use this specific "resource" will be reload. This parameter is not documented, for internal use only. PR elastic#96886 introduced auto-reload of analyzers on synonyms index change. The problem was that reloading was applied broady for all indices that contained reloadable analyzers. This PR improves this, so when a particular synonyms set changes, only analyzers that use this synonyms set will auto-reloaded. Note that shard requests will still be sent to all indices shards, as only on a shard we can decide if analyzers need to be reloaded.
mayya-sharipova
force-pushed
the
reload-analyzers-with-resource
branch
from
f68df16 to
30a2562
Compare
This PR adds a new optional parameter "resource" for ReloadAnalyzersRequest.
If used, only analyzers that use this specific "resource" will be reloaded.
This parameter is not documented, for internal use only.
PR #96886 introduced auto-reload of analyzers on synonyms index change. The problem
was that reloading was applied broadly for all indices that contained any reloadable
analyzers. This PR improves on it, so when a particular synonyms set changes,
only analyzers that use this synonyms set will auto-reloaded. Note that shard
requests will still be sent to all indices shards, as only on a shard we can
decide if analyzers need to be reloaded.