Move the percolator from core to its own module #18511
Conversation
martijnvg
added
>enhancement
>breaking
review
:Search Relevance/Percolator
| pluginsService.processModules(modulesBuilder); | ||
| injector = modulesBuilder.createInjector(); | ||
| IndexScopedSettings indexScopedSettings = injector.getInstance(IndexScopedSettings.class); | ||
| idxSettings = IndexSettingsModule.newIndexSettings(index, indexSettings, indexScopedSettings); |
There was a problem hiding this comment.
why is this moved down here and why do you need to Initialize idxSettings so late?
There was a problem hiding this comment.
I need it after the guice injector has been created, so that I can get IndexScopedSettings instance. The percolator query builder test that tests the doToQuery would otherwise fail, since there an index setting is checked that the PercolatorPlugin registers.
martijnvg
changed the title
martijnvg
force-pushed
the
percolator_module
branch
from
0438005
to
1ecb0af
Compare
| SearchContext.removeCurrent(); | ||
| } | ||
|
|
||
| protected final QB createTestQueryBuilder() { | ||
| static class Holder { |
There was a problem hiding this comment.
I couldn't come up with a better name...
|
@s1monw @cbuescher I've changed the initialization method to be a member method and introduced this the |
| @@ -420,36 +414,6 @@ | |||
| MultiTermVectorsRequestBuilder prepareMultiTermVectors(); | |||
|
|
|||
| /** | |||
| * Percolates a request returning the matches documents. | |||
| */ | |||
| ActionFuture<PercolateResponse> percolate(PercolateRequest request); | |||
There was a problem hiding this comment.
One thing that has come up with reindex being in a plugin: users of the Java API have trouble finding it. I don't know what, if anything, we should do about that. This will have the same problem.
There was a problem hiding this comment.
that is true. The docs that described how to integrate with percolate api via the Java client have been replaced with: https://www.elastic.co/guide/en/elasticsearch/client/java-api/master/java-specialized-queries.html#java-query-percolate-query
I'll bring back the percolator java api docs with a description on how to access the api now that the percolator api has been removed from the Client interface. Otherwise people that wish to use the percolator api a bit longer are in the dark before cutting over to the search api.
|
left some minors LGTM otherwise 👍 |
martijnvg
force-pushed
the
percolator_module
branch
from
1ecb0af
to
c6c9095
Compare
Significant changes: * AbstractQueryTestCase has moved to the test framework module, in order for query builder tests in modules and plugins * Added support to AbstractQueryTestCase to register plugins * Lift the restriction that only one percolator could be added per index. This validation existed in MapperService, but because the percolator moved to a module it could no longer exist there. Instead of bringing it back it was removed. This validation existed since the percolator cache only supported one percolator query per document, since the percolator cache has been removed this restriction could removed as well. * While moving percolator tests to the new module, also removed a couple of tests for the deprecated percolate and mpercolate api. These APIs are now sugar APIs for bwc and rediect to the searvh and msearvh APIs. Some tests were still testing as if percolate and mpercolate API did the percolation, but this no longer the case and these tests could be removed.
martijnvg
force-pushed
the
percolator_module
branch
from
c6c9095
to
27cc2fe
Compare
The most significant changes I had to make was moving
AbstractQueryTestCaseto the test framework module and add support the register custom plugins. Moving this base test class to the test framework was required because thepercolatequery is the first query outside of the core module and its unit testPercolateQueryBuilderTestsextends from this test base class. TheAbstractQueryTestCasedidn't have support the register queries outside of core, so added support to theAbstractQueryTestCaseclass to register a plugin which then register its custom queries if the plugin contains that. I'm not super happy with the way needed to expose this, the entire initialization happens in a static method, so tests that want to test custom query builders now need to have a static initializer that registers a plugin. If someone has a simpler/cleaner solution for this then that would be great.Other changes:
Note: The breaking part is that the percolate methods have been removed from the
Clientclass in the Java api.