Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3cc2298
Java 9: requiring elasticsearch from module-info.java fails
reda-alaoui Mar 13, 2018
bc1497d
Fix classes headers
reda-alaoui Mar 13, 2018
71c5c32
Add test module
reda-alaoui Mar 13, 2018
e4cac84
Add test module
reda-alaoui Mar 13, 2018
e0ec871
Remove empty descriptors
reda-alaoui Mar 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions server/src/main/java/org/apache/lucene/codecs/JigsawMarker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* 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.apache.lucene.codecs;

/**
* 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 {
}
26 changes: 26 additions & 0 deletions server/src/main/java/org/apache/lucene/search/suggest/document/JigsawMarker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* 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.apache.lucene.search.suggest.document;

/**
* 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 {
Copy link
Member

@jasontedor jasontedor Jan 30, 2019

Choose a reason for hiding this comment

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

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

}
Empty file removed server/src/main/resources/META-INF/services/org.apache.lucene.codecs.Codec
View file
Open in desktop
Empty file.
Empty file removed server/src/main/resources/META-INF/services/org.apache.lucene.codecs.DocValuesFormat
View file
Open in desktop
Empty file.
3 changes: 2 additions & 1 deletion settings.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ List projects = [
'test:fixtures:hdfs-fixture',
'test:fixtures:krb5kdc-fixture',
'test:fixtures:old-elasticsearch',
'test:logger-usage'
'test:logger-usage',
'test:jigsaw-consumer'
]

/**
Expand Down
44 changes: 44 additions & 0 deletions test/jigsaw-consumer/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* 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.
*/

apply plugin: 'elasticsearch.build'

sourceCompatibility = JavaVersion.VERSION_1_9
Copy link
Contributor

@alpar-t alpar-t Jan 14, 2019

Choose a reason for hiding this comment

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

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.

targetCompatibility = JavaVersion.VERSION_1_9

configurations.all {
transitive = false
}

dependencies {
compile "org.elasticsearch:elasticsearch:${version}"
}

ext.moduleName = 'jigsaw_consumer'

compileJava {
inputs.property("moduleName", moduleName)
doFirst {
options.compilerArgs = [
'--module-path', classpath.asPath,
'-sourcepath', 'src/main/java'
]
classpath = files()
}
}
21 changes: 21 additions & 0 deletions test/jigsaw-consumer/src/main/java/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* 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.
*/
module jigsaw_consumer {
requires elasticsearch;
Copy link
Contributor

@alpar-t alpar-t Jan 14, 2019

Choose a reason for hiding this comment

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

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 ).

Copy link

@tomdw tomdw Jan 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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' ?

Copy link
Contributor

@alpar-t alpar-t Jan 21, 2019

Choose a reason for hiding this comment

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

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.

Copy link

@tomdw tomdw Jan 26, 2019

Choose a reason for hiding this comment

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

@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?

Copy link

@tomdw tomdw Jan 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@alpar-t alpar-t Jan 28, 2019

Choose a reason for hiding this comment

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

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.

}
27 changes: 27 additions & 0 deletions test/jigsaw-consumer/src/main/java/org/elasticsearch/test/jigsaw_consumer/Foo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* 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.test.jigsaw_consumer;

import org.elasticsearch.client.Client;

public interface Foo {

Client client();

}