Skip to content

HBASE-26838 Junit jar is not included in the hbase tar ball, causing … #4223

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

Merged
wchevreuil merged 8 commits into from
Mar 31, 2022
Merged

HBASE-26838 Junit jar is not included in the hbase tar ball, causing … #4223

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1738871
HBASE-26838 Junit jar is not included in the hbase tar ball, causing …
wchevreuil Mar 14, 2022
ef79b4e
Removing blank lines in client.xml; Also moving hamcrest to lib/test
wchevreuil Mar 15, 2022
7f7266c
adding lib/test jars to IntegrationTestIngest classpath
wchevreuil Mar 21, 2022
be7c1e7
adding regex check for IntegrationTest in hbase script when deciding …
wchevreuil Mar 23, 2022
968cb3b
adding extra explanation on ref-guide; including mockito in the assem…
wchevreuil Mar 30, 2022
b570161
adding test libs on the client tar ball
wchevreuil Mar 30, 2022
fb87084
resetting scope test to parent pom and overriding it to compile in as…
wchevreuil Mar 31, 2022
1296a83
Adding comment on assembly pom explaining mockito compile scope
wchevreuil Mar 31, 2022
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
4 changes: 4 additions & 0 deletions hbase-assembly/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,5 +373,9 @@
<artifactId>opentelemetry-javaagent</artifactId>
<classifier>all</classifier>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
</dependency>
</dependencies>
</project>
2 changes: 2 additions & 0 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 @@ -52,6 +52,7 @@
<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>
Expand All @@ -64,6 +65,7 @@
<exclude>org.slf4j:*</exclude>
<exclude>org.apache.logging.log4j:*</exclude>
<exclude>io.opentelemetry.javaagent:*</exclude>
<exclude>org.hamcrest:hamcrest-core</exclude>
</excludes>
</dependencySet>
</dependencySets>
Expand Down
11 changes: 11 additions & 0 deletions hbase-assembly/src/main/assembly/hadoop-three-compat.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@
<exclude>org.slf4j:*</exclude>
<exclude>org.apache.logging.log4j:*</exclude>
<exclude>io.opentelemetry.javaagent:*</exclude>
<exclude>junit:junit</exclude>
<exclude>org.hamcrest:hamcrest-core</exclude>
</excludes>
</dependencySet>
</dependencySets>
Expand Down Expand Up @@ -261,6 +263,15 @@
<include>io.opentelemetry.javaagent:*</include>
</includes>
</dependencySet>

<!-- Adds junit libs to lib/test -->
<dependencySet>
<outputDirectory>lib/test</outputDirectory>
Copy link
Contributor

@Apache9 Apache9 Mar 15, 2022

Choose a reason for hiding this comment

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

Will we load the libraries under this package?

And better also include hamcrest here.

Copy link
Contributor Author

@wchevreuil wchevreuil Mar 15, 2022

Choose a reason for hiding this comment

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

Yes, these libraries would be packed under a "lib\test" folder, and would not be added to hbase classpath automatically (the hbase script, by default, puts "lib*" on the CP).

Copy link
Contributor

@Apache9 Apache9 Mar 15, 2022

Choose a reason for hiding this comment

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

I mean we should add these libraries?

Copy link
Contributor Author

@wchevreuil wchevreuil Mar 15, 2022

Choose a reason for hiding this comment

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

You mean add to hbase classpath by default? Junit used to be shipped until it was scoped for test only on all modules, at least since release 2.4 (In 2.2 it was still shipped). In general, HBase processes seem to work pretty fine without it on the CP. The only setback is for the test tooling, such as IntegrationTestIngest, which will now fail without junit lib in the classpath.

Copy link
Contributor

@Apache9 Apache9 Mar 16, 2022

Choose a reason for hiding this comment

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

So how do we setback when executing IntegrationTestIngest? Manually?

Copy link
Contributor Author

@wchevreuil wchevreuil Mar 17, 2022

Choose a reason for hiding this comment

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

Yes, this could be easily done as HBASE_CLASSPATH="$HBASE_HOME/lib/tests/*" hbase org.apache.hadoop.hbase.IntegrationTestIngest -m slowDeterministic, since we already provide the lib within the tarball. We could update the ref guide section accordingly.

Copy link
Contributor

@Apache9 Apache9 Mar 17, 2022

Choose a reason for hiding this comment

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

I wonder what is the problem if we always add these jars to our classpath? At lease when running tools(not daemon services)?

Copy link
Member

@joshelser joshelser Mar 20, 2022

Choose a reason for hiding this comment

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

Ehh, usually it's been a thing that causes people to ask questions rather than something actually breaking. Apparently others think that we shouldnt' have this on the $CLASSPATH given they removed them.

Copy link
Member

@ndimiduk ndimiduk Mar 21, 2022

Choose a reason for hiding this comment

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

For me, it is a security and release hygiene issue to ship test dependencies in the runtime. It is problematic for me that some of our user utilities have runtime dependency on test jars. Only mildly related: I'm curious how much extra space those test dependencies consume in our release artifacts.

Short of the restructuring that I suggested, I am okay with this approach of isolating these test dependencies in their own directory and conditionally including them at runtime. If there are classes that we know require them, our bin/hbase script can intelligently add them to the classpath for those classes. Doing so will preserve backward compatibility while also keeping the extra jars out of the daemon services' classpaths.

Copy link
Contributor Author

@wchevreuil wchevreuil Mar 21, 2022

Choose a reason for hiding this comment

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

Ok, let me add it to the CP of the tools that require it, in bin/hbase script.

Copy link
Member

@joshelser joshelser Mar 30, 2022

Choose a reason for hiding this comment

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

Just noticed this gets missed in the client tarball. Just copy this same block in hbase-assembly/src/main/assembly/client.xml

<includes>
<include>junit:junit</include>
<include>org.hamcrest:hamcrest-core</include>
</includes>
</dependencySet>
</dependencySets>

</assembly>