Java 9: requiring elasticsearch from module-info.java fails #29030

reda-alaoui · 2018-03-13T19:27:21Z

elasticmachine · 2018-03-13T19:27:23Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-03-13T19:27:23Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

jasontedor

Please remove author and date tags. Sorry, but these are a no-go. They are clutter at the top of the file, the information contained in them is available in commit history which is the single-source of truth for this information.

This does not constitute a review of the functionality being added, nor does it constitute and implicit "if you fix these we will pull this PR". Those will be considered separately.

jasontedor

Also, these are missing the license header.

elasticmachine · 2018-03-13T19:31:41Z

Pinging @elastic/es-core-infra

jasontedor

Another issue here is that there are no build checks being added to ensure that we do not break this again.

reda-alaoui · 2018-03-13T19:37:07Z

I don't see how to check that except by creating a full Java 9 module?
Do you want me to add a java9 gradle consumer module?

reda-alaoui · 2018-03-13T21:53:45Z

@jasontedor , I added a test module

reda-alaoui · 2018-03-13T23:04:20Z

The added test module doesn't fail compilation when changes are reverted ...
Maybe it is just a Maven issue :/

jasontedor · 2018-03-14T00:16:50Z

@reda-alaoui Thank you so much for pushing on this and adding a test, it really helps clarify the problem, ensure correctness, and would help us maintain this code if it were to be integrated. Since this is now appearing to be a Maven issue, I will put reviewing of this on hold.

reda-alaoui · 2018-03-14T00:23:55Z

@jasontedor , would it be ok to replace the test Gradle module by a Maven module?
I would trigger its build via a gradle exec task.

reda-alaoui · 2018-03-14T18:32:43Z

I am having issues with the added module because of gradle/gradle#3035.
I am not sure if I can disable compileJava fork in the submodule.

tomdw · 2018-08-10T07:59:11Z

@jasontedor as I understand it correctly it is not solely a maven issue, service providers have to be encapsulated in the module. Can the pr be merged to fix this to at least allow jdk 9 modules projects use elasticsearch on the module path as automatic module?

javanna · 2018-10-02T13:29:15Z

@jasontedor do we want to get this merged? Or has it become outdated and maybe no longer needed?

tomdw · 2018-10-02T18:37:07Z

@javanna certainly not outdated as it is still a problem to use elasticsearch libraries on the modulepath. This pr fixes that.

alpar-t · 2019-01-14T09:06:44Z

test/jigsaw-consumer/build.gradle

+
+apply plugin: 'elasticsearch.build'
+
+sourceCompatibility = JavaVersion.VERSION_1_9


The test folder is a collection of tools, and actual test projects live in qa, but for testing the build, I would move this to buildSrc/src/testKit and write a GradleIntegrationTestCase for it.
The source and target comparability can be parameters passed to the build so that we can test on newer than 9 also or control the Gradle version if we have too.

alpar-t · 2019-01-14T09:43:52Z

test/jigsaw-consumer/src/main/java/module-info.java

+ * under the License.
+ */
+module jigsaw_consumer {
+    requires elasticsearch;


Considering the comments in the issue that this will in fact fail at runtime, i'm concerned this might send the wrong message.

If I understand this right, you are hitting this problem because you move all classpath to the module path in your application to create automatic modules. You are probably only using a client of elasticsearch, but since elasticsearch is a dependency, it gets added as a module and causes issues.
On the other hand, where this would fail if I understand @uschindler right, is if one embeds elasticsearch in another application to make use of it's functionality, but this is not something we support.

I'm a bit confused about why you would require elasticsearch and not one of the clients.
Could you clarify that ? Is the requires necessary to reproduce the issue ?

I'm thinking that a more common use-case would be to depend and require on the client ( elasticsearch would be a transitive dependency and still be added as an automatic module ).

@atorok I think some types needed to use a client are in the elasticsearch jar, so an application still needs elasticsearch jar on its path even if it does not embed it as server. e.g. https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/index/query/QueryBuilders.java is in the elasticsearch jar and is used to build queries towards elasticsearch (same for org.elasticsearch.search.aggregations.AggregationBuilders)

which maven dependency are you referring to with 'client' ?

You are correct @tomdw , both org.elasticsearch.client:elasticsearch-rest-high-level-client and org.elasticsearch.client:transport depend on org.elasticsearch:elasticsearch. I'm assuming that an application would rather require one of those clients so it can talk to ES . I'm assuming you ended up having troubles with ES because you moved your classpath to module path, to make all jars automatic modules , which does include includes org.elasticsearch:elasticsearch as per the dependencies.

I think a test should rather have requires elasticsearch-rest-high-level-client; since this actually what users will have in their modules. requires elasticsearch; would be a line for when elasticsearch-rest-high-level-client would get a moule-info.
We could then go a step beyond ( not necessarily as part of this PR to have some code in the application that actually talks to a test cluster. I would expect the former to fail the same way without the fixes.

@atorok unfortunately code using the high level rest client still needs to requires elasticsearch; next to the requires elasticsearch-rest-high-level-client; because org.elasticsearch.index.query.QueryBuilder and others are in the elasticsearch jar and you can't call high level client APIs taking it as argument without also requiring the module containing org.elasticsearch.index.query.QueryBuilder

Would be better if the query building classes are extracted into their own module, something like 'elasticsearch-common-client' which is then used as shared module. Then a dependency on 'elasticsearch' might be avoidable.

Doesn't take away though that the 'elasticsearch' jar is not respecting the java spec by including META-INF/services/... files which point to non-existing packages and classes. When will this be fixed?

@atorok noticed another problem today: when you require elasticsearch-rest-high-level-client then you have a split package problem with elasticsearch.rest.client:

"reads package org.elasticsearch.client from both elasticsearch.rest.high.level.client and elasticsearch.rest.client"

which makes it impossible to use the high level rest client on the module path as automatic module

I'm not arguing against fixing the missing classes, but I want to make sure that the test we add will be a complete representation of how one might integrate an elasticsearch client into a jigsaw application so it can even serve as an example of how to do it.

MrGraversen · 2019-01-29T09:17:05Z

Hi @atorok and / or @jasontedor,

At my company we are currently waiting on this fix. Otherwise, we'd have to stand up a new "search service", which runs Java 8 or otherwise don't use the module system - and I'm not super inclined to go this route.

Could you share with us a possible timeline to get this merged, if you have it prioritised internally?

jasontedor · 2019-01-29T13:05:50Z

Could you share with us a possible timeline to get this merged, if you have it prioritised internally?

I am sorry but we do not offer such timelines for releases, nor specific features, nor specific bug fixes.

jasontedor

I left a comment about the change.

jasontedor · 2019-01-30T16:37:46Z

server/src/main/java/org/apache/lucene/search/suggest/document/JigsawMarker.java

+ * This marker is needed because Java 9 compiler expects the current package to contain at least one class during
+ * automodule descriptor derivation process.
+ */
+interface JigsawMarker {


I don't think these are necessary, we can remove the empty service files instead?

alpar-t · 2019-02-04T08:24:42Z

Thanks for your contribution @reda-alaoui but we decided to remove the empty service files (#38192) and address jigsaw support more broadly #38299.

Java 9: requiring elasticsearch from module-info.java fails

3cc2298

jasontedor requested changes Mar 13, 2018

View reviewed changes

jasontedor added the :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts label Mar 13, 2018

jasontedor requested changes Mar 13, 2018

View reviewed changes

jasontedor added :Delivery/Build Build or test infrastructure and removed :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts labels Mar 13, 2018

Fix classes headers

bc1497d

Add test module

71c5c32

Add test module

e4cac84

Remove empty descriptors

e0ec871

colings86 added the >non-issue label Apr 24, 2018

rjernst removed the >non-issue label Jul 11, 2018

alpar-t requested changes Jan 14, 2019

View reviewed changes

jasontedor requested changes Jan 30, 2019

View reviewed changes

alpar-t closed this Feb 4, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020


		apply plugin: 'elasticsearch.build'

		sourceCompatibility = JavaVersion.VERSION_1_9

Java 9: requiring elasticsearch from module-info.java fails #29030

Java 9: requiring elasticsearch from module-info.java fails #29030

Uh oh!

Conversation

reda-alaoui commented Mar 13, 2018

Uh oh!

elasticmachine commented Mar 13, 2018

Uh oh!

elasticmachine commented Mar 13, 2018

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Mar 13, 2018

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

reda-alaoui commented Mar 13, 2018

Uh oh!

reda-alaoui commented Mar 13, 2018

Uh oh!

reda-alaoui commented Mar 13, 2018

Uh oh!

jasontedor commented Mar 14, 2018

Uh oh!

reda-alaoui commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reda-alaoui commented Mar 14, 2018

Uh oh!

tomdw commented Aug 10, 2018

Uh oh!

javanna commented Oct 2, 2018

Uh oh!

tomdw commented Oct 2, 2018

Uh oh!

alpar-t Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

alpar-t Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

tomdw Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpar-t Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

tomdw Jan 26, 2019

Choose a reason for hiding this comment

Uh oh!

tomdw Jan 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpar-t Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

MrGraversen commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasontedor commented Jan 29, 2019

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor Jan 30, 2019

Choose a reason for hiding this comment

Uh oh!

alpar-t commented Feb 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

reda-alaoui commented Mar 14, 2018 •

edited

Loading

tomdw Jan 17, 2019 •

edited

Loading

tomdw Jan 27, 2019 •

edited

Loading

MrGraversen commented Jan 29, 2019 •

edited

Loading