-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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.
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-26838 Junit jar is not included in the hbase tar ball, causing … #4223
Changes from 6 commits
1738871
ef79b4e
7f7266c
be7c1e7
968cb3b
b570161
fb87084
1296a83
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
|
@@ -261,6 +264,16 @@ | |
<include>io.opentelemetry.javaagent:*</include> | ||
</includes> | ||
</dependencySet> | ||
|
||
<!-- Adds junit libs to lib/test --> | ||
<dependencySet> | ||
<outputDirectory>lib/test</outputDirectory> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean we should add these libraries? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So how do we setback when executing IntegrationTestIngest? Manually? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this could be easily done as There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
<includes> | ||
<include>junit:junit</include> | ||
<include>org.hamcrest:hamcrest-core</include> | ||
<include>org.mockito:mockito-core</include> | ||
</includes> | ||
</dependencySet> | ||
</dependencySets> | ||
|
||
</assembly> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2491,7 +2491,6 @@ | |
<groupId>org.mockito</groupId> | ||
<artifactId>mockito-core</artifactId> | ||
<version>${mockito-core.version}</version> | ||
<scope>test</scope> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you should not remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
</dependency> | ||
<dependency> | ||
<groupId>io.opentelemetry</groupId> | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.