Add precommit task for detecting split packages #73784
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Modularization of the JDK has been ongoing for several years. Recently in Java 16 the JDK began enforcing module boundaries by default. While Elasticsearch does not yet use the module system directly, there are some side effects even for those projects not modularized (eg elastic#73517). Before we can even begin to think about how to modularize, we must Prepare The Way by enforcing packages only exist in a single jar file, since the module system does not allow packages to coexist in multiple modules. This commit adds a precommit check to the build which detects split packages. The expectation is that we will add the existing split packages to the ignore list so that any new classes will not exacerbate the problem, and the work to cleanup these split packages can be parallelized. relates elastic#73525
Collaborator
|
Pinging @elastic/es-delivery (Team:Delivery) |
Member
Author
|
Note that this is a WIP as far as adding any existing ignores, but the Gradle work is ready to review. |
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Jun 4, 2021
The plugin classloader exists in its own jar file for legacy reasons, and while it should go away in the future, it currently duplicates the package name of the rest of the plugin classes. This commit moves the classloader into its own unique package. relates elastic#73784
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Jun 4, 2021
The org.elasticsearch.bootstrap package exists in server with classes for starting up Elasticsearch. The elasticsearch-core jar has a handful of classes that were split out from there, namely java version parsing and jarhell. This commit moves those classes to a new org.elasticsearch.jdk package so as to not split the server owned bootstrap package. relates elastic#73784
mark-vieira
requested changes
Jun 4, 2021
build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/BuildPlugin.java
Show resolved
Hide resolved
...main/java/org/elasticsearch/gradle/internal/precommit/SplitPackagesAuditPrecommitPlugin.java
Outdated
Show resolved
Hide resolved
...main/java/org/elasticsearch/gradle/internal/precommit/SplitPackagesAuditPrecommitPlugin.java
Outdated
Show resolved
Hide resolved
...main/java/org/elasticsearch/gradle/internal/precommit/SplitPackagesAuditPrecommitPlugin.java
Outdated
Show resolved
Hide resolved
...ternal/src/main/java/org/elasticsearch/gradle/internal/precommit/SplitPackagesAuditTask.java
Outdated
Show resolved
Hide resolved
...ternal/src/main/java/org/elasticsearch/gradle/internal/precommit/SplitPackagesAuditTask.java
Outdated
Show resolved
Hide resolved
...ternal/src/main/java/org/elasticsearch/gradle/internal/precommit/SplitPackagesAuditTask.java
Show resolved
Hide resolved
| return splitPackages; | ||
| } | ||
|
|
||
| private void filterSplitPackages(Map<String, Set<String>> splitPackages) { |
Contributor
There was a problem hiding this comment.
Goning back and mutating our findings seems unnecessarily complicated. Why not just filter these out directly in splitPackages(). Since we already have the package/classname there, we can just check against the filter list before adding to the result set.
Member
Author
There was a problem hiding this comment.
I prefer the separation, since the algorithms are clearer (how to find the split packages vs reading through filters).
...ternal/src/main/java/org/elasticsearch/gradle/internal/precommit/SplitPackagesAuditTask.java
Outdated
Show resolved
Hide resolved
rjernst
added a commit
that referenced
this pull request
Jun 7, 2021
The plugin classloader exists in its own jar file for legacy reasons, and while it should go away in the future, it currently duplicates the package name of the rest of the plugin classes. This commit moves the classloader into its own unique package. relates #73784
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Jun 7, 2021
The plugin classloader exists in its own jar file for legacy reasons, and while it should go away in the future, it currently duplicates the package name of the rest of the plugin classes. This commit moves the classloader into its own unique package. relates elastic#73784
rjernst
added a commit
that referenced
this pull request
Jun 7, 2021
The org.elasticsearch.bootstrap package exists in server with classes for starting up Elasticsearch. The elasticsearch-core jar has a handful of classes that were split out from there, namely java version parsing and jarhell. This commit moves those classes to a new org.elasticsearch.jdk package so as to not split the server owned bootstrap package. relates #73784
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Jun 7, 2021
The org.elasticsearch.bootstrap package exists in server with classes for starting up Elasticsearch. The elasticsearch-core jar has a handful of classes that were split out from there, namely java version parsing and jarhell. This commit moves those classes to a new org.elasticsearch.jdk package so as to not split the server owned bootstrap package. relates elastic#73784
rjernst
added a commit
that referenced
this pull request
Jun 7, 2021
The org.elasticsearch.bootstrap package exists in server with classes for starting up Elasticsearch. The elasticsearch-core jar has a handful of classes that were split out from there, namely java version parsing and jarhell. This commit moves those classes to a new org.elasticsearch.jdk package so as to not split the server owned bootstrap package. relates #73784
rjernst
added a commit
that referenced
this pull request
Jun 8, 2021
The plugin classloader exists in its own jar file for legacy reasons, and while it should go away in the future, it currently duplicates the package name of the rest of the plugin classes. This commit moves the classloader into its own unique package. relates #73784
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Jun 8, 2021
When libs/core was created, several classes were moved from server's o.e.common package, but they were not moved to a new package. Split packages need to go away long term, so that Elasticsearch can even think about modularization. This commit moves all the classes under o.e.common in core to o.e.core. relates elastic#73784
rjernst
added a commit
that referenced
this pull request
Jun 8, 2021
When libs/core was created, several classes were moved from server's o.e.common package, but they were not moved to a new package. Split packages need to go away long term, so that Elasticsearch can even think about modularization. This commit moves all the classes under o.e.common in core to o.e.core. relates #73784
Member
Author
|
@mark-vieira I think I've addressed all your comments, and I have added ignores for existing split packages. This should be ready for another look. |
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Jun 8, 2021
o.e.common package, but they were not moved to a new package. Split packages need to go away long term, so that Elasticsearch can even think about modularization. This commit moves all the classes under o.e.common in core to o.e.core. relates elastic#73784 backport elastic#73909
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Jun 8, 2021
ParseField is part of the x-content lib, yet it doesn't exist under the same root package as the rest of the lib. This commit moves the class to the appropriate package. relates elastic#73784
mark-vieira
approved these changes
Jun 8, 2021
Contributor
mark-vieira
left a comment
mark-vieira
left a comment
There was a problem hiding this comment.
LGTM, couple questions about comments referring to lucene but don't seem related.
plugins/analysis-icu/build.gradle
Outdated
| } | ||
|
|
||
| tasks.named('splitPackagesAudit').configure { | ||
| // Lucene packages should be owned by Lucene! |
Contributor
There was a problem hiding this comment.
What do these packages have to do with Lucene?
Member
Author
There was a problem hiding this comment.
This was a copy/paste error. I'll fix the comments.
| } | ||
|
|
||
| tasks.named('splitPackagesAudit').configure { | ||
| // Lucene packages should be owned by Lucene! |
Contributor
There was a problem hiding this comment.
Again, these are org.elasticsearch packages, how do they conflict with Lucene?
rjernst
added a commit
that referenced
this pull request
Jun 8, 2021
ParseField is part of the x-content lib, yet it doesn't exist under the same root package as the rest of the lib. This commit moves the class to the appropriate package. relates #73784
rjernst
added a commit
that referenced
this pull request
Jun 8, 2021
When libs/core was created, several classes were moved from server's o.e.common package, but they were not moved to a new package. Split packages need to go away long term, so that Elasticsearch can even think about modularization. This commit moves all the classes under o.e.common in core to o.e.core. relates #73784 backport #73909
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Jun 8, 2021
ParseField is part of the x-content lib, yet it doesn't exist under the same root package as the rest of the lib. This commit moves the class to the appropriate package. relates elastic#73784
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Jun 8, 2021
Modularization of the JDK has been ongoing for several years. Recently in Java 16 the JDK began enforcing module boundaries by default. While Elasticsearch does not yet use the module system directly, there are some side effects even for those projects not modularized (eg elastic#73517). Before we can even begin to think about how to modularize, we must Prepare The Way by enforcing packages only exist in a single jar file, since the module system does not allow packages to coexist in multiple modules. This commit adds a precommit check to the build which detects split packages. The expectation is that we will add the existing split packages to the ignore list so that any new classes will not exacerbate the problem, and the work to cleanup these split packages can be parallelized. relates elastic#73525
rjernst
added a commit
that referenced
this pull request
Jun 8, 2021
rjernst
added a commit
that referenced
this pull request
Jun 8, 2021
Modularization of the JDK has been ongoing for several years. Recently in Java 16 the JDK began enforcing module boundaries by default. While Elasticsearch does not yet use the module system directly, there are some side effects even for those projects not modularized (eg #73517). Before we can even begin to think about how to modularize, we must Prepare The Way by enforcing packages only exist in a single jar file, since the module system does not allow packages to coexist in multiple modules. This commit adds a precommit check to the build which detects split packages. The expectation is that we will add the existing split packages to the ignore list so that any new classes will not exacerbate the problem, and the work to cleanup these split packages can be parallelized. relates #73525
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Modularization of the JDK has been ongoing for several years. Recently
in Java 16 the JDK began enforcing module boundaries by default. While
Elasticsearch does not yet use the module system directly, there are
some side effects even for those projects not modularized (eg #73517).
Before we can even begin to think about how to modularize, we must
Prepare The Way by enforcing packages only exist in a single jar file,
since the module system does not allow packages to coexist in multiple
modules.
This commit adds a precommit check to the build which detects split
packages. The expectation is that we will add the existing split
packages to the ignore list so that any new classes will not exacerbate
the problem, and the work to cleanup these split packages can be
parallelized.
relates #73525