Make Multiplexer inherit filter chains analysis mode#50662
Merged
cbuescher merged 6 commits intoelastic:masterfrom Jan 8, 2020
Merged
Make Multiplexer inherit filter chains analysis mode#50662cbuescher merged 6 commits intoelastic:masterfrom
cbuescher merged 6 commits intoelastic:masterfrom
Conversation
Collaborator
|
Pinging @elastic/es-search (:Search/Analysis) |
Member
Author
|
@elasticmachine update branch |
romseygeek
requested changes
Jan 8, 2020
Contributor
romseygeek
left a comment
There was a problem hiding this comment.
Thanks @cbuescher, looks good; there's one more test that I think is worth adding.
| package org.elasticsearch.index.mapper; | ||
|
|
||
| import com.carrotsearch.hppc.ObjectHashSet; | ||
|
|
Contributor
There was a problem hiding this comment.
nit: I think this is a leftover?
Comment on lines
+126
to
+138
| String synonymsFileName = "synonyms.txt"; | ||
| Path configDir = node().getEnvironment().configFile(); | ||
| if (Files.exists(configDir) == false) { | ||
| Files.createDirectory(configDir); | ||
| } | ||
| Path synonymsFile = configDir.resolve(synonymsFileName); | ||
| if (Files.exists(synonymsFile) == false) { | ||
| Files.createFile(synonymsFile); | ||
| } | ||
| try (PrintWriter out = new PrintWriter( | ||
| new OutputStreamWriter(Files.newOutputStream(synonymsFile, StandardOpenOption.WRITE), StandardCharsets.UTF_8))) { | ||
| out.println("foo, baz"); | ||
| } |
Contributor
There was a problem hiding this comment.
Given that this is reused in three different tests now, I wonder if it's worth extracting it as a separate method?
| "Failed to parse mapping: analyzer [my_synonym_analyzer] " | ||
| + "contains filters [synonym_filter] that are not allowed to run in all mode.", | ||
| ex.getMessage()); | ||
| MapperException ex = expectThrows(MapperException.class, |
Contributor
There was a problem hiding this comment.
This compression of multiple settings onto single lines seems to me to make test more difficult to follow, can we keep the indentation as it was before?
Member
Author
There was a problem hiding this comment.
Good point, this was not intended, must be the new Eclipse installation doing some autoformatting.
| response = client().prepareSearch(indexName).setQuery(QueryBuilders.matchQuery("field", "buzz")).get(); | ||
| assertHitCount(response, 1L); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Maybe also add a test asserting that a multiplexer containing updateable synonyms is rejected as an index-time analyzer as well?
Member
Author
|
@romseygeek thanks for the review, your comments should be adressed now |
romseygeek
approved these changes
Jan 8, 2020
Contributor
romseygeek
left a comment
There was a problem hiding this comment.
LGTM, thanks @cbuescher!
cbuescher
pushed a commit
that referenced
this pull request
Jan 8, 2020
Currently, if an updateable synonym filter is included in a multiplexer filter, it is not reloaded via the _reload_search_analyzers because the multiplexer itself doesn't pass on the analysis mode of the filters it contains, so its not recognized as "updateable" in itself. Instead we can check and merge the AnalysisMode settings of all filters in the multiplexer and use the resulting mode (e.g. search-time only) for the multiplexer itself, thus making any synonym filters contained in it reloadable. This, of course, will also make the analyzers using the multiplexer be usable at search-time only. Closes #50554
SivagurunathanV
pushed a commit
to SivagurunathanV/elasticsearch
that referenced
this pull request
Jan 23, 2020
Currently, if an updateable synonym filter is included in a multiplexer filter, it is not reloaded via the _reload_search_analyzers because the multiplexer itself doesn't pass on the analysis mode of the filters it contains, so its not recognized as "updateable" in itself. Instead we can check and merge the AnalysisMode settings of all filters in the multiplexer and use the resulting mode (e.g. search-time only) for the multiplexer itself, thus making any synonym filters contained in it reloadable. This, of course, will also make the analyzers using the multiplexer be usable at search-time only. Closes elastic#50554
This was referenced Feb 3, 2020
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Currently, if an updateable synonym filter is included in a multiplexer filter, it is not reloaded
via the
_reload_search_analyzersbecause the multiplexer itself doesn't pass on the analysis modeof the filters it contains, so its not recognized as "updateable" in itself. Instead we can check and merge
the
AnalysisModesettings of all filters in the multiplexer and use the resulting mode (e.g. search-time only)for the multiplexer itself, thus making any synonym filters contained in it reloadable. This, of course, will also
make the analyzers using the multiplexer be usable at search-time only.
Marking this as WIP for now to get some test runs and for discussion.
Closes #50554