Skip to content
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

HBASE-29016 Refactor assembly creation to use only DependencySets and… #6519

Closed
wants to merge 6 commits into from
Closed

HBASE-29016 Refactor assembly creation to use only DependencySets and… #6519

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b2059c
HBASE-29016 Refactor assembly creation to use only DependencySets and…
stoty Dec 4, 2024
92b7780
comment changes
stoty Dec 6, 2024
e19a9a3
re-add jaxws-ri to help with review
stoty Dec 6, 2024
110f3dd
don't duplicate opentelemetry jars + cleanups
stoty Dec 6, 2024
c064868
re-add jax-ws pom exclusion temporarily
stoty Dec 6, 2024
94734e7
rename hbase-generate-dev-classpath to hbase-dev-generate-classpath
stoty Dec 9, 2024
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
125 changes: 34 additions & 91 deletions hbase-assembly/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,24 +47,6 @@
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-shaded-mapreduce</artifactId>
</dependency>
<!-- Intra-project dependencies -->
<!-- We don't really add this to assembly tarball, we retain it here just to dump it into
cached_classpath.txt ! See HBASE-28433 for more info. -->
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-it</artifactId>
<type>test-jar</type>
</dependency>
<!-- Hamcrest is required by hbase-it (via junit), but as long as we're grabbing the hbase-it test-jar,
maven dependency resolution won't pick it up for us. -->
<!-- We don't really add this to assembly tarball, we retain it here just to dump it into
cached_classpath.txt ! See HBASE-28433 for more info. -->
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest-core</artifactId>
<!-- Overriding the scope in depMgmt -->
<scope>compile</scope>
</dependency>
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-openssl</artifactId>
Expand All @@ -75,27 +57,11 @@
</dependency>
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-mapreduce</artifactId>
<artifactId>hbase-endpoint</artifactId>
</dependency>
<!-- We don't really add this to assembly tarball, we retain it here just to dump it into
cached_classpath.txt ! See HBASE-28433 for more info. -->
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-mapreduce</artifactId>
<type>test-jar</type>
</dependency>
<!-- To dump tools in hbase-procedure into cached_classpath.txt. -->
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-procedure</artifactId>
</dependency>
<!-- We don't really add this to assembly tarball, we retain it here just to dump it into
cached_classpath.txt ! See HBASE-28433 for more info. -->
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-procedure</artifactId>
<type>test-jar</type>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.hbase</groupId>
Expand Down Expand Up @@ -123,12 +89,6 @@
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-external-blockcache</artifactId>
</dependency>
<!-- We don't really add this to assembly tarball, we retain it here just to dump it into
cached_classpath.txt ! See HBASE-28433 for more info. -->
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-testing-util</artifactId>
</dependency>
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-metrics-api</artifactId>
Expand All @@ -154,6 +114,10 @@
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpcore</artifactId>
</dependency>
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-asyncfs</artifactId>
</dependency>
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-backup</artifactId>
Expand Down Expand Up @@ -206,6 +170,8 @@
<groupId>jline</groupId>
<artifactId>jline</artifactId>
</dependency>
<!-- FIXME this is only kept to simplify reviewing the assembly refactor.
See HBASE-29010 -->
<dependency>
<groupId>com.sun.xml.ws</groupId>
<artifactId>jaxws-ri</artifactId>
Expand Down Expand Up @@ -245,20 +211,36 @@
<groupId>io.opentelemetry.javaagent</groupId>
<artifactId>opentelemetry-javaagent</artifactId>
</dependency>
<!-- We don't really add this to assembly tarball, we retain it here just to dump it into
cached_classpath.txt ! See HBASE-28433 for more info. -->
<!-- This is an optional dependency of hbase-external-blockcache.
Copy link
Contributor

Choose a reason for hiding this comment

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

If so, let's just remove the optional flag in hbase-external-blockcache's pom file?

Copy link
Contributor Author

@stoty stoty Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I took another look at this.

I guess that the original intent was to NOT include it in the assembly, it just got lost somehow.

The last version is also from 2017, so this doesn't look a maintaned library.

Based on the comments, getting this right is far from trivial, and this probably has few users, so it might be better to keep it optional, and just leave it out from assembly.

That way most users will get less JARs, and the few (if any) users of it would have to provide it themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we can do the removal in another ticket, so that it's better documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have opened a [DISCUSS] thread for spymemcached removal.

The assembly traditionally includes it, so we add it as an explicit dependency -->
<dependency>
<groupId>net.spy</groupId>
<artifactId>spymemcached</artifactId>
</dependency>
<!-- The tomcat JSP Compiler dependencies are not needed for runtime -->
<dependency>
<groupId>org.apache.tomcat</groupId>
<artifactId>tomcat-juli</artifactId>
<version>${tomcat.version.for.exclusion}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.apache.tomcat</groupId>
<artifactId>tomcat-api</artifactId>
<version>${tomcat.version.for.exclusion}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<groupId>org.apache.tomcat</groupId>
<artifactId>tomcat-util-scan</artifactId>
<version>${tomcat.version.for.exclusion}</version>
<scope>provided</scope>
</dependency>
<!-- We don't really add this to assembly tarball, we retain it here just to dump it into
cached_classpath.txt ! See HBASE-28433 for more info. -->
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<!-- Making it compile here so that it can be downloaded during packaging. -->
<!-- All other modules scope it as test or inherit test scope from parent pom. -->
<scope>compile</scope>
<groupId>org.apache.tomcat</groupId>
<artifactId>tomcat-util</artifactId>
<version>${tomcat.version.for.exclusion}</version>
<scope>provided</scope>
</dependency>
</dependencies>
<build>
Expand Down Expand Up @@ -313,45 +295,6 @@
<plugin>
<artifactId>maven-dependency-plugin</artifactId>
<executions>
<execution>
<!-- generates the file that will be used by the bin/hbase script in the dev env -->
<id>create-hbase-generated-classpath</id>
<goals>
<goal>build-classpath</goal>
</goals>
<phase>test</phase>
<configuration>
<outputFile>${project.parent.basedir}/target/cached_classpath.txt</outputFile>
<excludeArtifactIds>jline,jruby-complete,hbase-shaded-client,hbase-shaded-client-byo-hadoop,hbase-shaded-mapreduce</excludeArtifactIds>
</configuration>
</execution>

<execution>
<!-- generates the file that will be used by the bin/hbase zkcli script in the dev env -->
<id>create-hbase-generated-classpath-jline</id>
<goals>
<goal>build-classpath</goal>
</goals>
<phase>test</phase>
<configuration>
<outputFile>${project.parent.basedir}/target/cached_classpath_jline.txt</outputFile>
<includeArtifactIds>jline</includeArtifactIds>
</configuration>
</execution>

<execution>
<!-- generates the file that will be used by the bin/hbase shell script in the dev env -->
<id>create-hbase-generated-classpath-jruby</id>
<goals>
<goal>build-classpath</goal>
</goals>
<phase>test</phase>
<configuration>
<outputFile>${project.parent.basedir}/target/cached_classpath_jruby.txt</outputFile>
<includeArtifactIds>jruby-complete</includeArtifactIds>
</configuration>
</execution>

<!--
Build an aggregation of our templated NOTICE file and the NOTICE files in our dependencies.
If MASSEMBLY-382 is fixed we could do this in the assembly
Expand Down
111 changes: 52 additions & 59 deletions hbase-assembly/src/main/assembly/client.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,55 +31,10 @@
<baseDirectory>hbase-${project.version}-client</baseDirectory>
<componentDescriptors>
<componentDescriptor>src/main/assembly/client-components.xml</componentDescriptor>
<!-- Not that everything below is an exact copy of hadoop-three-compat.xml
Unless differences are introduced on purpose, this should be kept in sync with
hadoop-three-compat.xml -->
</componentDescriptors>
<moduleSets>
<!-- include regular jars so the shell can use them -->
<moduleSet>
<useAllReactorProjects>true</useAllReactorProjects>
<includes>
<include>org.apache.hbase:hbase-shell</include>
</includes>
<binaries>
<unpack>false</unpack>
<outputDirectory>lib</outputDirectory>
<dependencySets>
<dependencySet>
<excludes>
<exclude>org.apache.hadoop:*:test-jar</exclude>
<exclude>org.apache.hbase:*:test-jar</exclude>
<!-- Exclude J2EE libraries that get pulled in when building on JDK11 -->
<exclude>com.sun.xml.ws:jaxws-ri</exclude>
<!-- Exclude libraries that we put in their own dirs under lib/ -->
<exclude>org.jruby:jruby-complete</exclude>
<exclude>com.sun.jersey:*</exclude>
<exclude>com.sun.jersey.contribs:*</exclude>
<exclude>jline:jline</exclude>
<exclude>junit:junit</exclude>
<exclude>com.github.stephenc.findbugs:findbugs-annotations</exclude>
<exclude>commons-logging:commons-logging</exclude>
<exclude>log4j:log4j</exclude>
<exclude>org.apache.hbase:hbase-shaded-client</exclude>
<exclude>org.apache.hbase:hbase-shaded-client-byo-hadoop</exclude>
<exclude>org.apache.hbase:hbase-shaded-mapreduce</exclude>
<exclude>org.apache.htrace:htrace-core4</exclude>
<exclude>org.apache.htrace:htrace-core</exclude>
<exclude>org.apache.yetus:audience-annotations</exclude>
<exclude>org.slf4j:*</exclude>
<exclude>org.apache.logging.log4j:*</exclude>
<exclude>io.opentelemetry.javaagent:*</exclude>
<exclude>org.hamcrest:hamcrest-core</exclude>
<exclude>org.mockito:mockito-core</exclude>
<!-- Exclude transitive dependencies of tomcat-jasper, not needed at runtime -->
<exclude>org.apache.tomcat:tomcat-juli</exclude>
<exclude>org.apache.tomcat:tomcat-api</exclude>
<exclude>org.apache.tomcat:tomcat-util-scan</exclude>
<exclude>org.apache.tomcat:tomcat-util</exclude>
</excludes>
</dependencySet>
</dependencySets>
</binaries>
</moduleSet>
</moduleSets>
<!-- Include the generated LICENSE and NOTICE files -->
<files>
<file>
Expand All @@ -104,24 +59,47 @@

<dependencySets>
<dependencySet>
<outputDirectory>lib/shaded-clients</outputDirectory>
<includes>
<include>org.apache.hbase:hbase-shaded-client</include>
<include>org.apache.hbase:hbase-shaded-mapreduce</include>
<include>org.apache.hbase:hbase-shaded-client-byo-hadoop</include>
</includes>
<outputDirectory>lib</outputDirectory>
<useTransitiveDependencies>true</useTransitiveDependencies>
Copy link
Contributor

@NihalJain NihalJain Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nit: <!-- Exclude artifacts added in the sub-directories to avoid duplication -->

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Synced to the other xml

<!-- Exclude artifacts added in the sub-directories to avoid duplication -->
<excludes>
<!-- Exclude the shaded jars that go in the lib/shaded-clients directory -->
<exclude>org.apache.hbase:hbase-shaded-client</exclude>
<exclude>org.apache.hbase:hbase-shaded-mapreduce</exclude>
<exclude>org.apache.hbase:hbase-shaded-client-byo-hadoop</exclude>
<!-- Exclude the Ruby jar that goes in the lib/ruby directory -->
<exclude>org.jruby:jruby-complete</exclude>
<!-- Exclude jars that go into the lib/client-facing-thirdparty directory -->
<exclude>com.github.stephenc.findbugs:findbugs-annotations</exclude>
Copy link
Contributor

@NihalJain NihalJain Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

so the excludes block should be kept in sync with any dependencies we add in a sub folder of lib. may be add a note somewhere to avoid dependencies getting duplicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

<exclude>commons-logging:commons-logging</exclude>
<exclude>log4j:log4j</exclude>
<exclude>org.apache.htrace:htrace-core4</exclude>
<exclude>org.apache.htrace:htrace-core</exclude>
<exclude>org.apache.yetus:audience-annotations</exclude>
<exclude>org.slf4j:*</exclude>
<exclude>org.apache.logging.log4j:*</exclude>
<!-- TODO shouldn't we also exclude duplicate io.opentelemetry.* jars which are added to client-facing-thirdparty ? -->
Copy link
Contributor

@NihalJain NihalJain Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @stoty any reason for not excluding io.opentelemetry.* here?

Copy link
Contributor Author

@stoty stoty Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wasn't 100% sure that the current version doesn't do this on purpose, and I wanted to minimize the changes.

We can add the exclusion here, or we can open a follow-up ticket for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sound good either ways

<!-- Exclude the opentelemetry agent that goes in the lib/trace directory -->
<exclude>io.opentelemetry.javaagent:*</exclude>
<!-- Exclude jline2 which goes into lib/zk-client. jline3 is a different artifact-->
<exclude>jline:jline</exclude>
<!-- These two exclusions are added here to preserve previous behaviour, and are not included in sub-directories -->
<!-- TODO investigate using the provided mechanism for these -->
<exclude>com.sun.jersey:*</exclude>
<exclude>com.sun.jersey.contribs:*</exclude>
<!-- FIXME remove when jaxws-ri is removed from assembly -->
<exclude>com.sun.xml.ws:jaxws-ri:pom</exclude>
</excludes>
</dependencySet>
<!-- Add jruby-complete to hbase_home/lib/ruby.
Update JRUBY_PACKAGED_WITH_HBASE in bin/hbase and hbase.cmd if you would like to update outputDirectory below -->
<!-- Add jruby-complete to hbase_home/lib/ruby.
Update JRUBY_PACKAGED_WITH_HBASE in bin/hbase and hbase.cmd if you would like to update outputDirectory below -->
<dependencySet>
<outputDirectory>lib/ruby</outputDirectory>
<includes>
<include>org.jruby:jruby-complete</include>
</includes>
</dependencySet>
<!-- Include third party dependencies the shaded clients expose in the lib directory
N.B. this will conflict with the omnibus tarball but these should be precisely
the same artifacts so blind overwrite of either should be fine.
-->
<dependencySet>
<outputDirectory>lib/client-facing-thirdparty</outputDirectory>
Expand Down Expand Up @@ -152,6 +130,7 @@
tarball in a different build.
-->
<includes>
<!-- TODO Review and remove entries that are not present in any supported HBase and Hadoop version -->
<include>com.github.stephenc.findbugs:findbugs-annotations</include>
<include>commons-logging:commons-logging</include>
<include>log4j:log4j</include>
Expand All @@ -163,12 +142,26 @@
<include>io.opentelemetry:*</include>
</includes>
</dependencySet>
<dependencySet>
<outputDirectory>lib/shaded-clients</outputDirectory>
<includes>
<include>org.apache.hbase:hbase-shaded-client</include>
<include>org.apache.hbase:hbase-shaded-mapreduce</include>
<include>org.apache.hbase:hbase-shaded-client-byo-hadoop</include>
</includes>
</dependencySet>
<dependencySet>
<outputDirectory>lib/zkcli</outputDirectory>
<includes>
<!-- This is jline2 -->
<include>jline:jline</include>
</includes>
</dependencySet>
<dependencySet>
<outputDirectory>lib/trace</outputDirectory>
<includes>
<include>io.opentelemetry.javaagent:*</include>
</includes>
</dependencySet>
</dependencySets>

</assembly>
Loading