From 65c7f61ad9ebc3ae7993373583377962ce02ecad Mon Sep 17 00:00:00 2001 From: javanna Date: Thu, 8 Sep 2016 19:15:28 +0200 Subject: [PATCH] decouple registration of SearchExtParsers from sub fetch phases Search section supports an ext section that is used to provide additional config needed from plugins. It is now tied to sub fetch phases because it is the only section that may need additional config, but there is no reason for the two to be tightly coupled. It is now possible to register a searchExtParser independently from a sub fetch phase. All a search ext parser does is parsing some ext section of a search request, whose parsed resulting object is stored in the search context for later retrieval. --- .../elasticsearch/plugins/SearchPlugin.java | 7 ++++ .../search/SearchExtParserRegistry.java | 32 +++++++++++++++++++ .../elasticsearch/search/SearchModule.java | 11 +++++++ .../elasticsearch/search/SearchService.java | 27 ++++++---------- .../search/fetch/FetchPhase.java | 12 ------- .../search/fetch/FetchSubPhase.java | 8 ----- .../search/internal/DefaultSearchContext.java | 10 +++--- .../internal/FilteredSearchContext.java | 8 ++--- .../search/internal/SearchContext.java | 4 +-- .../search/fetch/FetchSubPhasePluginIT.java | 12 +++---- .../search/MockSearchService.java | 5 +-- .../elasticsearch/test/TestSearchContext.java | 10 +++--- 12 files changed, 84 insertions(+), 62 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/search/SearchExtParserRegistry.java diff --git a/core/src/main/java/org/elasticsearch/plugins/SearchPlugin.java b/core/src/main/java/org/elasticsearch/plugins/SearchPlugin.java index 97ee715ef6a..a01837509c0 100644 --- a/core/src/main/java/org/elasticsearch/plugins/SearchPlugin.java +++ b/core/src/main/java/org/elasticsearch/plugins/SearchPlugin.java @@ -30,6 +30,7 @@ import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryParser; import org.elasticsearch.index.query.functionscore.ScoreFunctionBuilder; import org.elasticsearch.index.query.functionscore.ScoreFunctionParser; +import org.elasticsearch.search.SearchExtParser; import org.elasticsearch.search.aggregations.Aggregation; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.Aggregator; @@ -82,6 +83,12 @@ public interface SearchPlugin { default List getFetchSubPhases(FetchPhaseConstructionContext context) { return emptyList(); } + /** + * The new {@link SearchExtParser}s defined by this plugin. + */ + default List getSearchExtParsers() { + return emptyList(); + } /** * Get the {@link Highlighter}s defined by this plugin. */ diff --git a/core/src/main/java/org/elasticsearch/search/SearchExtParserRegistry.java b/core/src/main/java/org/elasticsearch/search/SearchExtParserRegistry.java new file mode 100644 index 00000000000..423ae802874 --- /dev/null +++ b/core/src/main/java/org/elasticsearch/search/SearchExtParserRegistry.java @@ -0,0 +1,32 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.search; + +import org.elasticsearch.common.xcontent.ParseFieldRegistry; + +/** + * Extensions to ParseFieldRegistry to make Guice happy. + */ +public class SearchExtParserRegistry extends ParseFieldRegistry { + + SearchExtParserRegistry() { + super("ext"); + } +} diff --git a/core/src/main/java/org/elasticsearch/search/SearchModule.java b/core/src/main/java/org/elasticsearch/search/SearchModule.java index ab36f03639c..60cfdf7ab5b 100644 --- a/core/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/core/src/main/java/org/elasticsearch/search/SearchModule.java @@ -306,6 +306,7 @@ public class SearchModule extends AbstractModule { "moving_avg_model"); private final List fetchSubPhases = new ArrayList<>(); + private final SearchExtParserRegistry searchExtParserRegistry = new SearchExtParserRegistry(); private final Settings settings; private final List namedWriteables = new ArrayList<>(); @@ -326,6 +327,7 @@ public class SearchModule extends AbstractModule { registerAggregations(plugins); registerPipelineAggregations(plugins); registerFetchSubPhases(plugins); + registerSearchExtParsers(plugins); registerShapes(); searchRequestParsers = new SearchRequestParsers(queryParserRegistry, aggregatorParsers, getSuggesters()); } @@ -380,6 +382,7 @@ public class SearchModule extends AbstractModule { if (false == transportClient) { bind(IndicesQueriesRegistry.class).toInstance(queryParserRegistry); bind(SearchRequestParsers.class).toInstance(searchRequestParsers); + bind(SearchExtParserRegistry.class).toInstance(searchExtParserRegistry); configureSearch(); } } @@ -725,6 +728,14 @@ public class SearchModule extends AbstractModule { registerFromPlugin(plugins, p -> p.getFetchSubPhases(context), this::registerFetchSubPhase); } + private void registerSearchExtParsers(List plugins) { + registerFromPlugin(plugins, SearchPlugin::getSearchExtParsers, this::registerSearchExtParser); + } + + private void registerSearchExtParser(SearchExtParser searchExtParser) { + searchExtParserRegistry.register(searchExtParser, searchExtParser.getName()); + } + private void registerFetchSubPhase(FetchSubPhase subPhase) { Class subPhaseClass = subPhase.getClass(); if (fetchSubPhases.stream().anyMatch(p -> p.getClass().equals(subPhaseClass))) { diff --git a/core/src/main/java/org/elasticsearch/search/SearchService.java b/core/src/main/java/org/elasticsearch/search/SearchService.java index 09c1bda2d8d..96f0629beb9 100644 --- a/core/src/main/java/org/elasticsearch/search/SearchService.java +++ b/core/src/main/java/org/elasticsearch/search/SearchService.java @@ -100,7 +100,6 @@ import java.util.Optional; import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicLong; -import static java.util.Collections.unmodifiableMap; import static org.elasticsearch.common.unit.TimeValue.timeValueMillis; import static org.elasticsearch.common.unit.TimeValue.timeValueMinutes; @@ -143,13 +142,14 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv private final ConcurrentMapLong activeContexts = ConcurrentCollections.newConcurrentMapLongWithAggressiveConcurrency(); - private final Map fetchPhaseParsers; + private final SearchExtParserRegistry searchExtParserRegistry; private final ParseFieldMatcher parseFieldMatcher; @Inject public SearchService(Settings settings, ClusterSettings clusterSettings, ClusterService clusterService, IndicesService indicesService, - ThreadPool threadPool, ScriptService scriptService, BigArrays bigArrays, FetchPhase fetchPhase) { + ThreadPool threadPool, ScriptService scriptService, BigArrays bigArrays, FetchPhase fetchPhase, + SearchExtParserRegistry searchExtParserRegistry) { super(settings); this.parseFieldMatcher = new ParseFieldMatcher(settings); this.threadPool = threadPool; @@ -163,10 +163,10 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv TimeValue keepAliveInterval = KEEPALIVE_INTERVAL_SETTING.get(settings); this.defaultKeepAlive = DEFAULT_KEEPALIVE_SETTING.get(settings).millis(); - this.fetchPhaseParsers = unmodifiableMap(fetchPhase.parsers()); - this.keepAliveReaper = threadPool.scheduleWithFixedDelay(new Reaper(), keepAliveInterval, Names.SAME); + this.searchExtParserRegistry = searchExtParserRegistry; + defaultSearchTimeout = DEFAULT_SEARCH_TIMEOUT_SETTING.get(settings); clusterSettings.addSettingsUpdateConsumer(DEFAULT_SEARCH_TIMEOUT_SETTING, this::setDefaultSearchTimeout); } @@ -762,19 +762,10 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = extParser.currentName(); } else { - SearchExtParser searchExtParser = this.fetchPhaseParsers.get(currentFieldName); - if (searchExtParser == null) { - if (currentFieldName != null && currentFieldName.equals("suggest")) { - throw new SearchParseException(context, - "suggest is not supported in [ext], please use SearchSourceBuilder#suggest(SuggestBuilder) instead", - extParser.getTokenLocation()); - } - throw new SearchParseException(context, "Unknown element [" + currentFieldName + "] in [ext]", - extParser.getTokenLocation()); - } else { - Object fetchSubPhaseBuilder = searchExtParser.parse(extParser); - context.putFetchSubPhaseBuilder(currentFieldName, fetchSubPhaseBuilder); - } + SearchExtParser searchExtParser = this.searchExtParserRegistry.lookup(currentFieldName, parseFieldMatcher, + extParser.getTokenLocation()); + Object searchExtBuilder = searchExtParser.parse(extParser); + context.putSearchExtBuilder(currentFieldName, searchExtBuilder); } } } catch (Exception e) { diff --git a/core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java b/core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java index b45ae23e0bd..41ea0e294da 100644 --- a/core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java +++ b/core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java @@ -41,7 +41,6 @@ import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.ObjectMapper; import org.elasticsearch.index.mapper.SourceFieldMapper; -import org.elasticsearch.search.SearchExtParser; import org.elasticsearch.search.SearchHit; import org.elasticsearch.search.SearchHitField; import org.elasticsearch.search.SearchPhase; @@ -62,7 +61,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import static java.util.Collections.unmodifiableMap; import static org.elasticsearch.common.xcontent.XContentFactory.contentBuilder; /** @@ -78,16 +76,6 @@ public class FetchPhase implements SearchPhase { this.fetchSubPhases[fetchSubPhases.size()] = new InnerHitsFetchSubPhase(this); } - public Map parsers() { - Map parsers = new HashMap<>(); - for (FetchSubPhase fetchSubPhase : fetchSubPhases) { - if (fetchSubPhase.parser() != null) { - parsers.put(fetchSubPhase.parser().getName(), fetchSubPhase.parser()); - } - } - return unmodifiableMap(parsers); - } - @Override public void preProcess(SearchContext context) { } diff --git a/core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.java b/core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.java index 1873568f8d4..1783652d120 100644 --- a/core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.java +++ b/core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.java @@ -22,7 +22,6 @@ import org.apache.lucene.index.IndexReader; import org.apache.lucene.index.LeafReader; import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.search.IndexSearcher; -import org.elasticsearch.search.SearchExtParser; import org.elasticsearch.search.internal.InternalSearchHit; import org.elasticsearch.search.internal.SearchContext; @@ -77,13 +76,6 @@ public interface FetchSubPhase { } - /** - * Returns the parser for the optional config to be put in the ext section of the search request - */ - default SearchExtParser parser() { - return null; - } - /** * Executes the hit level phase, with a reader and doc id (note, its a low level reader, and the matching doc). */ diff --git a/core/src/main/java/org/elasticsearch/search/internal/DefaultSearchContext.java b/core/src/main/java/org/elasticsearch/search/internal/DefaultSearchContext.java index 35f472da46d..5aacb633416 100644 --- a/core/src/main/java/org/elasticsearch/search/internal/DefaultSearchContext.java +++ b/core/src/main/java/org/elasticsearch/search/internal/DefaultSearchContext.java @@ -145,7 +145,7 @@ public class DefaultSearchContext extends SearchContext { private volatile long lastAccessTime = -1; private Profilers profilers; - private final Map subPhaseBuilders = new HashMap<>(); + private final Map searchExtBuilders = new HashMap<>(); private final Map, Collector> queryCollectors = new HashMap<>(); private final QueryShardContext queryShardContext; private FetchPhase fetchPhase; @@ -385,13 +385,13 @@ public class DefaultSearchContext extends SearchContext { } @Override - public Object getFetchSubPhaseBuilder(String name) { - return subPhaseBuilders.get(name); + public Object getSearchExtBuilder(String name) { + return searchExtBuilders.get(name); } @Override - public void putFetchSubPhaseBuilder(String name, Object fetchSubPhaseBuilder) { - subPhaseBuilders.put(name, fetchSubPhaseBuilder); + public void putSearchExtBuilder(String name, Object searchExtBuilder) { + searchExtBuilders.put(name, searchExtBuilder); } @Override diff --git a/core/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java b/core/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java index afd14ce07ab..c9d10a7aa67 100644 --- a/core/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java +++ b/core/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java @@ -510,13 +510,13 @@ public abstract class FilteredSearchContext extends SearchContext { } @Override - public void putFetchSubPhaseBuilder(String name, Object fetchSubPhaseBuilder) { - in.putFetchSubPhaseBuilder(name, fetchSubPhaseBuilder); + public void putSearchExtBuilder(String name, Object fetchSubPhaseBuilder) { + in.putSearchExtBuilder(name, fetchSubPhaseBuilder); } @Override - public Object getFetchSubPhaseBuilder(String name) { - return in.getFetchSubPhaseBuilder(name); + public Object getSearchExtBuilder(String name) { + return in.getSearchExtBuilder(name); } @Override diff --git a/core/src/main/java/org/elasticsearch/search/internal/SearchContext.java b/core/src/main/java/org/elasticsearch/search/internal/SearchContext.java index 4520c866602..e060669fd5c 100644 --- a/core/src/main/java/org/elasticsearch/search/internal/SearchContext.java +++ b/core/src/main/java/org/elasticsearch/search/internal/SearchContext.java @@ -184,9 +184,9 @@ public abstract class SearchContext extends AbstractRefCounted implements Releas public abstract SearchContext aggregations(SearchContextAggregations aggregations); - public abstract void putFetchSubPhaseBuilder(String name, Object fetchSubPhaseBuilder); + public abstract void putSearchExtBuilder(String name, Object fetchSubPhaseBuilder); - public abstract Object getFetchSubPhaseBuilder(String name); + public abstract Object getSearchExtBuilder(String name); public abstract SearchContextHighlight highlight(); diff --git a/core/src/test/java/org/elasticsearch/search/fetch/FetchSubPhasePluginIT.java b/core/src/test/java/org/elasticsearch/search/fetch/FetchSubPhasePluginIT.java index 4dbdc850706..2517f89ec49 100644 --- a/core/src/test/java/org/elasticsearch/search/fetch/FetchSubPhasePluginIT.java +++ b/core/src/test/java/org/elasticsearch/search/fetch/FetchSubPhasePluginIT.java @@ -103,19 +103,19 @@ public class FetchSubPhasePluginIT extends ESIntegTestCase { public List getFetchSubPhases(FetchPhaseConstructionContext context) { return singletonList(new TermVectorsFetchSubPhase()); } + + @Override + public List getSearchExtParsers() { + return Collections.singletonList(TermVectorsFetchParser.INSTANCE); + } } public static final class TermVectorsFetchSubPhase implements FetchSubPhase { private static final String NAME = "term_vectors_fetch"; - @Override - public SearchExtParser parser() { - return TermVectorsFetchParser.INSTANCE; - } - @Override public void hitExecute(SearchContext context, HitContext hitContext) { - TermVectorsFetchBuilder fetchSubPhaseBuilder = (TermVectorsFetchBuilder)context.getFetchSubPhaseBuilder(NAME); + TermVectorsFetchBuilder fetchSubPhaseBuilder = (TermVectorsFetchBuilder)context.getSearchExtBuilder(NAME); if (fetchSubPhaseBuilder == null) { return; } diff --git a/test/framework/src/main/java/org/elasticsearch/search/MockSearchService.java b/test/framework/src/main/java/org/elasticsearch/search/MockSearchService.java index cae5b2ff95b..832a85654c1 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/MockSearchService.java +++ b/test/framework/src/main/java/org/elasticsearch/search/MockSearchService.java @@ -72,8 +72,9 @@ public class MockSearchService extends SearchService { @Inject public MockSearchService(Settings settings, ClusterSettings clusterSettings, ClusterService clusterService, IndicesService indicesService, ThreadPool threadPool, ScriptService scriptService, - BigArrays bigArrays, FetchPhase fetchPhase) { - super(settings, clusterSettings, clusterService, indicesService, threadPool, scriptService, bigArrays, fetchPhase); + BigArrays bigArrays, FetchPhase fetchPhase, SearchExtParserRegistry searchExtParserRegistry) { + super(settings, clusterSettings, clusterService, indicesService, threadPool, scriptService, + bigArrays, fetchPhase, searchExtParserRegistry); } @Override diff --git a/test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java b/test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java index a520b144df1..e9976b93657 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java +++ b/test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java @@ -89,7 +89,7 @@ public class TestSearchContext extends SearchContext { private SearchContextAggregations aggregations; private final long originNanoTime = System.nanoTime(); - private final Map subPhaseContexts = new HashMap<>(); + private final Map searchExtBuilders = new HashMap<>(); public TestSearchContext(ThreadPool threadPool, BigArrays bigArrays, ScriptService scriptService, IndexService indexService) { super(ParseFieldMatcher.STRICT); @@ -196,13 +196,13 @@ public class TestSearchContext extends SearchContext { } @Override - public void putFetchSubPhaseBuilder(String name, Object fetchSubPhaseBuilder) { - subPhaseContexts.put(name, fetchSubPhaseBuilder); + public void putSearchExtBuilder(String name, Object searchExtBuilders) { + this.searchExtBuilders.put(name, searchExtBuilders); } @Override - public Object getFetchSubPhaseBuilder(String name) { - return subPhaseContexts.get(name); + public Object getSearchExtBuilder(String name) { + return searchExtBuilders.get(name); } @Override