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

Jump to bottom

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

Merged
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 all 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
7 changes: 7 additions & 0 deletions bin/hbase
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,13 @@ elif [ "$COMMAND" = "hbtop" ] ; then
HBASE_OPTS="${HBASE_OPTS} ${HBASE_HBTOP_OPTS}"
else
CLASS=$COMMAND
if [[ "$CLASS" =~ .*IntegrationTest.* ]] ; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ndimiduk , I believe all classes on hbase-it which are exposed for running standalone follow this naming pattern. These are spread over few different packages.

Copy link
Member

Choose a reason for hiding this comment

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

There's quite a few more classes than that.

$ grep 'public static void main' -cirIF hbase-it/src --include *.java | grep -v IntegrationTest | grep -v Action | wc -l
      46

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I had noticed before that there were few others with a main method, but the only ones really relying on junit are the "IntegrationTest" ones.

for f in ${HBASE_HOME}/lib/test/*.jar; do
if [ -f "${f}" ]; then
CLASSPATH="${CLASSPATH}:${f}"
fi
done
fi
fi

# Add lib/jdk11 jars to the classpath
Expand Down
11 changes: 11 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,16 @@
<artifactId>opentelemetry-javaagent</artifactId>
<classifier>all</classifier>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
</dependency>
<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>
Copy link
Member

Choose a reason for hiding this comment

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

This works? Great!

Because compile is the default scope, someone later may be tempted to remove this distinction. Maybe add a comment to future do-gooders?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

</dependency>
</dependencies>
</project>
12 changes: 12 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,8 @@
<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>
</excludes>
</dependencySet>
</dependencySets>
Expand Down Expand Up @@ -159,6 +162,15 @@
<include>io.opentelemetry.javaagent:*</include>
</includes>
</dependencySet>
<!-- Adds junit libs to lib/test -->
<dependencySet>
<outputDirectory>lib/test</outputDirectory>
<includes>
<include>junit:junit</include>
<include>org.hamcrest:hamcrest-core</include>
<include>org.mockito:mockito-core</include>
</includes>
</dependencySet>
</dependencySets>

</assembly>
13 changes: 13 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,9 @@
<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>
<exclude>org.mockito:mockito-core</exclude>
</excludes>
</dependencySet>
</dependencySets>
Expand Down Expand Up @@ -261,6 +264,16 @@
<include>io.opentelemetry.javaagent:*</include>
</includes>
</dependencySet>

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

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

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

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

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

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

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

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

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

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

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

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>
<include>org.mockito:mockito-core</include>
</includes>
</dependencySet>
</dependencySets>

</assembly>
13 changes: 12 additions & 1 deletion src/main/asciidoc/_chapters/developer.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1575,7 +1575,7 @@ They are generally long-lasting, sizeable (the test can be asked to 1M rows or 1
Integration tests are what you would run when you need to more elaborate proofing of a release candidate beyond what unit tests can do.
They are not generally run on the Apache Continuous Integration build server, however, some sites opt to run integration tests as a part of their continuous testing on an actual cluster.

Integration tests currently live under the _src/test_ directory in the hbase-it submodule and will match the regex: _**/IntegrationTest*.java_.
Integration tests currently live under the _src/test_ directory in the hbase-it submodule and will match the regex: _*IntegrationTest*.java_.
All integration tests are also annotated with `@Category(IntegrationTests.class)`.

Integration tests can be run in two modes: using a mini cluster, or against an actual distributed cluster.
Expand Down Expand Up @@ -1692,6 +1692,17 @@ Currently tarball deployments, deployments which uses _hbase-daemons.sh_, and li
_/etc/init.d/_ scripts are not supported for now, but it can be easily added.
For other deployment options, a ClusterManager can be implemented and plugged in.

Some integration tests define a _main_ method as entry point, and can be run on its' own, rather than using the test driver. For example, the _itbll_ test can be run as follows:

----
bin/hbase org.apache.hadoop.hbase.test.IntegrationTestBigLinkedList loop 2 1 100000 /temp 1 1000 50 1 0
----

NOTE: The _hbase_ script assumes all integration tests with exposed _main_ methods to be run
against a distributed cluster will follow the *IntegrationTest* regex naming pattern
mentioned above, in order to proper set test dependencies into the classpath.


[[maven.build.commands.integration.tests.destructive]]
==== Destructive integration / system tests (ChaosMonkey)

Expand Down