Skip to content

Conversation

@tlrx
Copy link
Member

@tlrx tlrx commented Jun 7, 2017

The High Level REST Client can use Java's SPI to load named XContent parsers implementations provided by plugins or modules.

This pull request is an alternate solution for #25024 after a suggestion from @rjernst and requires #25097 to be merged first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean that we don't distribute the modules by default with the high level client?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we could avoid to ship them by default and let users add the JAR files to the classpath if they need them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. This goes against the notion of modules. If we ship them by default with core, we should also make them available by default with the client?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @javanna. This functionality is in the ES distribution, therefore the client should have it accessible with no extra work. However, I also see the benefit of keeping the core client lightweight, and having extra functionality added optionally. But I think that could be discussed and debated separately from this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted that change.

@rjernst
Copy link
Member

rjernst commented Jun 8, 2017

The changes look ok as a start to this. I think we will want to think more through interfaces, how/if plugins can implement through SPI as well, etc (which can be done as followups as necessary). I do not understand though why the spi classes are in a different PR, it should just be part of this one.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM let's get this in, but let's restore shipping the client with parent-join and matrix-stats at least for now.

@rjernst
Copy link
Member

rjernst commented Jun 8, 2017

@tlrx Can you please fold the PR adding spi classes and tests into this one? They are not useful as PRs on their own.

@tlrx
Copy link
Member Author

tlrx commented Jun 8, 2017

@rjernst Done.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still LGTM

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this much better as well.

tlrx added 4 commits June 15, 2017 10:51
The High Level REST Client can use Java's SPI to load named XContent
parsers implementations provided by plugins or modules.
@tlrx tlrx force-pushed the use-spi-to-load-named-xcontent-parsers branch from 3d1f589 to 9e8d469 Compare June 15, 2017 08:52
@tlrx tlrx merged commit 27f1206 into elastic:master Jun 15, 2017
@tlrx
Copy link
Member Author

tlrx commented Jun 15, 2017

Thanks all for your reviews

@tlrx
Copy link
Member Author

tlrx commented Jun 15, 2017

The changes look ok as a start to this. I think we will want to think more through interfaces, how/if plugins can implement through SPI as well, etc (which can be done as followups as necessary).

@rjernst If you have already any ideas around this, I'll be happy to hear them and help to implement.

tlrx added a commit that referenced this pull request Jun 15, 2017
This commit adds a NamedXContentProvider interface that can
be implemented by plugins or modules using Java's SPI feature
in order to provide additional NamedXContent parsers to external
applications like the Java High Level Rest Client.
@tlrx tlrx deleted the use-spi-to-load-named-xcontent-parsers branch June 15, 2017 11:12
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jun 15, 2017
* master:
  Upgrade icu4j for the ICU analysis plugin to 59.1 (elastic#25243)
  move assertBusy to use CheckException (elastic#25246)
  Use SPI in High Level Rest Client to load XContent parsers (elastic#25098)
  [TEST] test that low level REST client leaves path untouched (elastic#25193)
  Speed up PK lookups at index time. (elastic#19856)
  [Docs] Fix documentation for percentiles bucket aggregation (elastic#25229)
  Upgrade to lucene-7.0.0-snapshot-92b1783. (elastic#25222)
  Build: Add master flag for disabling bwc tests (elastic#25230)
  Scripting: Rename SearchScript.needsScores to needs_score (elastic#25235)
  Support script context stateful factory in Painless. (elastic#25233)
  FastVectorHighlighter should not cache the field query globally (elastic#25197)
  Remove QUERY_AND_FETCH BWC for pre-5.3.0 nodes (elastic#25223)
  Add more missing AggregationBuilder getters (elastic#25198)
  Extract the snapshot/restore full cluster restart tests from the translog full cluster restart tests (elastic#25204)
jasontedor added a commit to glefloch/elasticsearch that referenced this pull request Jun 15, 2017
* master: (44 commits)
  Upgrade icu4j for the ICU analysis plugin to 59.1 (elastic#25243)
  move assertBusy to use CheckException (elastic#25246)
  Use SPI in High Level Rest Client to load XContent parsers (elastic#25098)
  [TEST] test that low level REST client leaves path untouched (elastic#25193)
  Speed up PK lookups at index time. (elastic#19856)
  [Docs] Fix documentation for percentiles bucket aggregation (elastic#25229)
  Upgrade to lucene-7.0.0-snapshot-92b1783. (elastic#25222)
  Build: Add master flag for disabling bwc tests (elastic#25230)
  Scripting: Rename SearchScript.needsScores to needs_score (elastic#25235)
  Support script context stateful factory in Painless. (elastic#25233)
  FastVectorHighlighter should not cache the field query globally (elastic#25197)
  Remove QUERY_AND_FETCH BWC for pre-5.3.0 nodes (elastic#25223)
  Add more missing AggregationBuilder getters (elastic#25198)
  Extract the snapshot/restore full cluster restart tests from the translog full cluster restart tests (elastic#25204)
  Refactor TransportShardBulkAction.executeUpdateRequest and add tests
  Make sure range queries are correctly profiled. (elastic#25108)
  Test: allow setting socket timeout for rest client (elastic#25221)
  Migration docs for elastic#25080 (elastic#25218)
  Remove `discovery.type` BWC layer from the EC2/Azure/GCE plugins elastic#25080
  When stopping via systemd only kill the JVM, not its control group (elastic#25195)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants