HBASE-29016 Refactor assembly creation to use only DependencySets and… by stoty · Pull Request #6519 · apache/hbase

stoty · 2024-12-04T15:01:33Z

… move cached classpath creation to a new module

stoty · 2024-12-05T15:13:48Z

I have tested this on a pseudo-distributed cluster, starting and stopping HBase, and running a simple smoke test via HBase shell, and the rowCounter MR job.

stoty · 2024-12-05T15:14:31Z

+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.hbase</groupId>
+ <artifactId>hbase-build-configuration</artifactId>


Better names welcome

may be hbase-dev-generate-classpath? so that we can prefix all dev related modules as "hbase-dev-*" in case we add more in future

I will hold off changing that in case we get other suggestions.

… move cached classpath creation to a new module also remove explicit jaxsw-ri dependency from assembly

NihalJain · 2024-12-06T05:52:55Z

+ <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 ? -->


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

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.

Sound good either ways

NihalJain · 2024-12-06T05:54:12Z

+ <!-- 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 directoy -->
+ <exclude>com.github.stephenc.findbugs:findbugs-annotations</exclude>


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?

NihalJain · 2024-12-06T06:00:56Z

 </files>

 <dependencySets>
+ <dependencySet>


Is client.xml and hadoop-three-compat a copy now?

Yes.
The differences were already minimal, I think they were accidental, too.

Sure, we can see if we should keep both as follow up. I am good as long as generated assembly is same

NihalJain · 2024-12-06T06:02:29Z

+ <artifactId>hbase-mapreduce</artifactId>
+ <type>test-jar</type>
+ </dependency>
+ <!-- To dump tools in hbase-procedure into cached_classpath.txt. -->


this comment is no longer needed right?

ah we are referring why we addded hbase-procedure here. ignore comment

Actually, I think that this comment IS redundant, I don't see why hbase-procedure would be different than the rest of the artifacts.

I think that some of the scope changes, etc are also unneccessary in this case, but I ran out steam when cleaning up the pom.

NihalJain · 2024-12-06T06:12:40Z

Is there any difference between tarball generated with this change and without this change? If both are same, a +1 from me, as long as we all are fine to move to use only DependencySets, this is lot cleaner and avoids mixing up.

Although it is important to note that this step comes with a extra maintenance step of excluding any non base folder directory jar. But I am fine with this given we barely add new sub folders and we already plan to add a note in code to notify devs to take care of this.

stoty · 2024-12-06T06:19:36Z

Yes, there are changes,.
I have added a list of the changes with analysis to the JIRA.

About half of them is the jaxws-ri removal, which could be split into a different issue, if desired.

NihalJain · 2024-12-06T06:27:19Z

Yes, there are changes,. I have added a list of the changes with analysis to the JIRA.
About half of them is the jaxws-ri removal, which could be split into a different issue, if desired.

It would prefer to have it separate it it is not too much effort since if we do more this jira should be otherwise "Refactor and cleanup"

Also thinking about the future use case of having assembly with assembly-without-hadoop-jars, have you thought how the dependencySet mechanism will be able to handle it? Would we need to create a new module for assembly-without-hadoop-jars now? As now I am not sure how we can support multiple assemblies with single dependency set without managing inclusion/exclusion lists for each assembly.xml. Or do we plan to follow inclusion/exclusion list approach only.

stoty · 2024-12-06T06:27:56Z

I have also added some reasoning on why we need the assembly changes to the JIRA.

NihalJain · 2024-12-06T06:30:48Z

I have also added some reasoning on why we need the assembly changes to the JIRA.

Just saw the JIRA title, sounds good to me.

stoty · 2024-12-06T06:41:42Z

For the hadoop-less assembly, I plan to copy this assembly, and add the Hadoop artifacts as provided scope dependencies.

That is the best (only) way I know to remove both the Hadoop artifacts, and their exclusive transitive dependencies, while keeping HBase's transitive dependencies.

The only other way I know would be excluding and including everything by hand, which would be super fragile.

stoty · 2024-12-06T06:50:41Z

Re-added jaxws-ri

NihalJain · 2024-12-06T06:59:49Z

- <include>org.apache.hbase:hbase-shaded-client-byo-hadoop</include>
- </includes>
+ <outputDirectory>lib</outputDirectory>
+ <useTransitiveDependencies>true</useTransitiveDependencies>


nit: 

Synced to the other xml

NihalJain

+1 LGTM

stoty · 2024-12-06T07:07:34Z

I have checked the external and internal classpaths, and determined that duplicating the opentelemtry JARs is not needed.
The shaded classpaths don't care, and the internal classpath had duplicated copies.

stoty · 2024-12-06T07:07:48Z

So I removed the duplication.

Apache9 · 2024-12-09T08:30:39Z

 </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.


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

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.

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

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

Apache9 · 2024-12-09T08:39:36Z

Let's have a try first.

Apache-HBase · 2024-12-09T13:38:51Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 35s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 1s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
			_ master Compile Tests _
+0 🆗	mvndep	0m 30s		Maven dependency ordering for branch
+1 💚	mvninstall	4m 19s		master passed
+1 💚	compile	10m 12s		master passed
+1 💚	spotless	0m 54s		branch has no errors when running spotless:check.
-0 ⚠️	patch	1m 9s		Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 18s		Maven dependency ordering for patch
+1 💚	mvninstall	3m 43s		the patch passed
+1 💚	compile	9m 33s		the patch passed
+1 💚	javac	9m 33s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	xmllint	0m 0s		No new issues.
+1 💚	hadoopcheck	13m 19s		Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚	spotless	0m 54s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 34s		The patch does not generate ASF License warnings.
		52m 59s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6519/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#6519
Optional Tests	dupname asflicense javac codespell detsecrets xmllint hadoopcheck spotless compile
uname	Linux a29ca77ca737 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `94734e7`
Default Java	Eclipse Adoptium-17.0.11+9
Max. process+thread count	188 (vs. ulimit of 30000)
modules	C: hbase-assembly hbase-dev-generate-classpath . U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6519/4/console
versions	git=2.34.1 maven=3.9.8 xmllint=20913
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

stoty · 2025-01-09T14:22:18Z

I have already merged this back in December.

This comment has been minimized.
Sign in to view

stoty requested review from Apache9, NihalJain and ndimiduk December 5, 2024 14:29

stoty commented Dec 5, 2024
View reviewed changes

HBASE-29016 Refactor assembly creation to use only DependencySets and…
9b2059c
… move cached classpath creation to a new module also remove explicit jaxsw-ri dependency from assembly

stoty force-pushed the HBASE-29016 branch from 1300aa6 to 9b2059cCompare December 6, 2024 05:51

NihalJain reviewed Dec 6, 2024
View reviewed changes

comment changes
92b7780

re-add jaxws-ri to help with review
e19a9a3

This comment has been minimized.
Sign in to view

NihalJain reviewed Dec 6, 2024
View reviewed changes

NihalJain approved these changes Dec 6, 2024
View reviewed changes

don't duplicate opentelemetry jars + cleanups
110f3dd

re-add jax-ws pom exclusion temporarily
c064868

This comment has been minimized.
Sign in to view

Apache9 approved these changes Dec 9, 2024
View reviewed changes

rename hbase-generate-dev-classpath to hbase-dev-generate-classpath
94734e7

stoty closed this Jan 9, 2025

Uh oh!

Conversation

stoty commented Dec 4, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

stoty commented Dec 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NihalJainDec 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NihalJainDec 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stotyDec 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NihalJainDec 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NihalJain commented Dec 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stoty commented Dec 6, 2024

Uh oh!

NihalJain commented Dec 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stoty commented Dec 6, 2024

Uh oh!

NihalJain commented Dec 6, 2024

Uh oh!

stoty commented Dec 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stoty commented Dec 6, 2024

Uh oh!

This comment has been minimized.

NihalJainDec 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NihalJain left a comment

Choose a reason for hiding this comment

Uh oh!

stoty commented Dec 6, 2024

Uh oh!

stoty commented Dec 6, 2024

Uh oh!

This comment has been minimized.

NihalJainDec 6, 2024•
edited
Loading

NihalJainDec 6, 2024•
edited
Loading

stotyDec 6, 2024•
edited
Loading

NihalJainDec 6, 2024•
edited
Loading

NihalJain commented Dec 6, 2024•
edited
Loading

NihalJain commented Dec 6, 2024•
edited
Loading

stoty commented Dec 6, 2024•
edited
Loading

NihalJainDec 6, 2024•
edited
Loading

stotyDec 9, 2024•
edited
Loading