Allow RestHighLevelClient to use plugins #25024
Conversation
This commit changes the RestHighLevelClient class so that it now accepts a list of plugin classes as a constructor parameter. Similarly to the PreBuiltTransportClient, the RestHighLevelClient uses the PluginService to load the plugins and then extracts their aggregations and suggesters specs. The aggregation specs have been changed in core in order to add a `addResultParser` method that allows to reference the parsing methods of an aggregation. This information is use in the RestHighLevelClient to register additional NamedXContent entries to parse back custom aggregations/suggesters.
|
I'm not sure we should do this. This adds a tight coupling (the plugins service) between ES core and the high level client. Can we instead use java's mechanism for this, which is SPI? |
I'm not sure I like the idea of introducing another plugin lookup mechanism but I agree with Ryan that we should try not to introduce this level of coupling. I'd like to be able to reason about the Plugin API being internal to Elasticsearch. |
Its not "another", it is the original. SPI is perfect for this type of thing. Note that I am not suggesting a "plugin" be loaded through SPI, but instead named xcontent parsers. Then there is no need for an instance of the client to pass anything in, it is automatically loaded from the classpath by including these dependencies in your application classpath. |
|
@rjernst I agree with you that SPI is probably the way to go. It is totally another lookup mechanism because we don't use it now. I'm not talking about more code. I'm talking about more things to understand. SPI swaps one set of issues for another. |
| @@ -160,23 +165,52 @@ | |||
| * Can be sub-classed to expose additional client methods that make use of endpoints added to Elasticsearch through plugins, or to | |||
| * add support for custom response sections, again added to Elasticsearch through plugins. | |||
| */ | |||
| @SuppressWarnings("varargs") | |||
There was a problem hiding this comment.
Nit: curious about what this does, it shows up as "Unsupported @SuppressWarnings" in Eclipse
| } | ||
|
|
||
| /** | ||
| * Creates a {@link RestHighLevelClient} given the low level {@link RestClient} that it should use to perform requests and | ||
| * a list of entries that allow to parse custom response sections added to Elasticsearch through plugins. | ||
| */ | ||
| protected RestHighLevelClient(RestClient restClient, List<NamedXContentRegistry.Entry> namedXContentEntries) { | ||
| private RestHighLevelClient(RestClient restClient, List<NamedXContentRegistry.Entry> namedXContentEntries) { |
There was a problem hiding this comment.
Nit: Could this be moved into the ctor where it is called now?
| Collection<Class<? extends Plugin>> plugins) { | ||
| List<Class<? extends Plugin>> listOfPlugins = new ArrayList<>(preInstalledPlugins); | ||
| for (Class<? extends Plugin> plugin : plugins) { | ||
| if (listOfPlugins.contains(plugin)) { |
There was a problem hiding this comment.
Just out of curiosity: plugin equality here is exact object equality, since plugins are singletons, no?
| @@ -555,4 +587,27 @@ static boolean convertExistsResponse(Response response) { | |||
| (parser, context) -> CompletionSuggestion.fromXContent(parser, (String)context))); | |||
| return entries; | |||
| } | |||
|
|
|||
| static List<NamedXContentRegistry.Entry> pluginsNamedXContents(Settings settings, | |||
| @@ -542,8 +576,6 @@ static boolean convertExistsResponse(Response response) { | |||
| map.put(SignificantLongTerms.NAME, (p, c) -> ParsedSignificantLongTerms.fromXContent(p, (String) c)); | |||
| map.put(SignificantStringTerms.NAME, (p, c) -> ParsedSignificantStringTerms.fromXContent(p, (String) c)); | |||
| map.put(ScriptedMetricAggregationBuilder.NAME, (p, c) -> ParsedScriptedMetric.fromXContent(p, (String) c)); | |||
| map.put(ChildrenAggregationBuilder.NAME, (p, c) -> ParsedChildren.fromXContent(p, (String) c)); | |||
| map.put(MatrixStatsAggregationBuilder.NAME, (p, c) -> ParsedMatrixStats.fromXContent(p, (String) c)); | |||
There was a problem hiding this comment.
Why are these removed? Should the user have to specify them as client plugins explicitely, even if they are bundled with core by default?
There was a problem hiding this comment.
I think I understand now, is it because they are part of PRE_INSTALLED_PLUGINS?
|
|
||
| TermSuggestion suggestion = new TermSuggestion(suggestEntry.getKey(), customNumber, SortBy.SCORE); | ||
| for (int i = 0; i < customNumber; i++) { | ||
| suggestion.addTerm(new TermSuggestion.Entry(new Text("term " + i), i, customNumber)); |
There was a problem hiding this comment.
Nice, I just figured out how you check the response by preparing it here.
| @@ -262,11 +267,33 @@ public AggregationSpec addResultReader(String writeableName, Writeable.Reader<? | |||
| } | |||
|
|
|||
| /** | |||
| * Get the readers that must be registered for this aggregation's results. | |||
| * Get the readers that can be used to parse this aggregation's results when it has been printed out as a {@link ToXContent}. | |||
There was a problem hiding this comment.
I think this comment is off, the method still returns the readers for the transport level. This is the right comment for getResultParsers() below though.
| /** | ||
| * Adds a {@link ContextParser} that can be used to parse this aggregation when it has been printed out as a {@link ToXContent}. | ||
| */ | ||
| public AggregationSpec addResultParser(String typeName, ContextParser<Object, ? extends ParsedAggregation> resultParser) { |
There was a problem hiding this comment.
This currently seems ony to be used by the other addResultParser() method, I assume this is still public so that plugin authors can have different ways of providing the name of the aggregation?
| * Get the parsers that can be used to parse this aggregation's results when it has been printed out as a {@link ToXContent}. | ||
| */ | ||
| public Map<String, ContextParser<Object, ? extends ParsedAggregation>> getResultParsers() { | ||
| return resultParsers; |
| @@ -48,7 +48,7 @@ public final String getName() { | |||
| return name; | |||
| } | |||
|
|
|||
| protected void setName(String name) { | |||
| public void setName(String name) { | |||
There was a problem hiding this comment.
Nit: Should still work as protected as far as I can see.
|
Thanks @cbuescher @rjernst @nik9000 for your feedback. I agree this pull request introduces a tight coupling between the high level rest client and the PluginService and I didn't really like too while I was coding it (but I do like the similitude between TransportClient and RestHighLevelClient instantiation with plugin classes). I just didn't think of SPI at all and it's a good suggestion. I could revisit this pull request so that it uses SPI to load implementations of a core interface (let's call it I can open another pull request in core to create this NamedXContentProvider interface (that basically has 1 method |
|
I'd probably let this PR sit and open competing PR that uses the SPI. It doesn't have to do all the plugins, just a one or two to make it clear that it'd work. |
|
I think that we need to extract the plugin test out of this PR, all the rest is in other PRs, right? |
This commit changes the
RestHighLevelClientclass so that it now accepts a list of plugin classes as a constructor parameter. Similarly to thePreBuiltTransportClient, the RestHighLevelClient uses the PluginService to load the plugins and then extracts their aggregations and suggesters specs. The aggregation specs have been changed in core in order to add aaddResultParsermethod that allows to reference the parsing methods of an aggregation. This information is use in the RestHighLevelClientto register additional NamedXContent entries to parse back custom aggregations/suggesters.