HBASE-28883 Manage hbase-thirdparty transitive dependencies via BOM pom by NihalJain · Pull Request #124 · apache/hbase-thirdparty · GitHub

No description provided.

Also, please see code change required in HBase to consume this: NihalJain/hbase@0b51c71

NOTE: Could not remove the hardcoding of internal.protobuf.version as with BOM seems its not possible to pass on properties.

Have kept relevant code still here in this PR. Will cleanup based on review.

CC: @ndimiduk

@Apache9 mentioned in https://github.com/apache/hbase/pull/6295/files#r1775002522, we may not need to bump netty4 as it is for zk/hadoop. Same for error prone.

We can decide what all dependencies we would want to keep here. Here's a exhaustive list of dependencies i found relevant.

Why is the flatten plugin used for the bom? By definition, there should be no dependencies via this pom.

Hi @ndimiduk To preserve the dependencyManagement section in the bom, I had to choose flatten mode: bom. We have flattenMode=oss in parent but for the bom we need to have flattenMode=bom. Hence, I had to add this in order to override the flatten mode defined in parent.

Please see https://www.mojohaus.org/flatten-maven-plugin/apidocs/org/codehaus/mojo/flatten/FlattenMode.html

Why is this one included? There's no mention of it in the hbase.git/pom

We do have this at https://github.com/apache/hbase/blob/master/pom.xml#L1555
Although I am not sure if we need to keep this in sync.

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 14s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
-0 ⚠️	test4tests	0m 0s	The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ master Compile Tests _
+1 💚	mvninstall	0m 48s	master passed
+1 💚	compile	0m 14s	master passed
+1 💚	javadoc	0m 7s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 7s	Maven dependency ordering for patch
+1 💚	mvninstall	0m 41s	the patch passed
+1 💚	compile	0m 19s	the patch passed
+1 💚	javac	0m 19s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	xml	0m 1s	The patch has no ill-formed XML file.
+1 💚	javadoc	0m 11s	the patch passed
		_ Other Tests _
+1 💚	unit	0m 5s	hbase-thirdparty-bom in the patch passed.
+1 💚	unit	0m 33s	root in the patch passed.
+1 💚	asflicense	0m 11s	The patch does not generate ASF License warnings.
		4m 41s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hbase.apache.org/job/HBase-Thirdparty-PreCommit/job/PR-124/3/artifact/yetus-precommit-check/output/Dockerfile
GITHUB PR	#124
Optional Tests	dupname asflicense javac javadoc unit xml compile
uname	Linux ced608014c2e 5.4.0-192-generic #212-Ubuntu SMP Fri Jul 5 09:47:39 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
git revision	master / a98a74b
Default Java	Temurin-1.8.0_422-b05
Test Results	https://ci-hbase.apache.org/job/HBase-Thirdparty-PreCommit/job/PR-124/3/testReport/
Max. process+thread count	394 (vs. ulimit of 1000)
modules	C: hbase-thirdparty-bom . U: .
Console output	https://ci-hbase.apache.org/job/HBase-Thirdparty-PreCommit/job/PR-124/3/console
versions	git=2.43.0 maven=3.9.9
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

Looks alright by me. How does it work? have you tried a PR vs. the main repo that makes use of this?

Actually should we have logging dependencies here?

Looks alright by me. How does it work? have you tried a PR vs. the main repo that makes use of this?

Hi @ndimiduk Please refer apache/hbase#6366

Actually should we have logging dependencies here?

Yes we can technically have all dependencies we want to keep common across our projects. Please let me know if we would like to add log4j and other logging jars over here. Also please let me know if we should evaluate other libs which can be moved to this bom, for eg: junit, mockito, hamcrest etc?

we should not have this as per observation in apache/hbase#6366 (comment)

CC: @ndimiduk

This PR was blocked due to open comment at apache/hbase#6366 (comment)

CC: @ndimiduk

NihalJain marked this pull request as draft September 25, 2024 16:08

NihalJain commented Sep 25, 2024

View reviewed changes

Comment thread pom.xml Outdated

NihalJain commented Sep 25, 2024

View reviewed changes

NihalJain mentioned this pull request Sep 25, 2024

HBASE-28879 Bump hbase-thirdparty to 4.1.9 apache/hbase#6295

Merged

This comment has been minimized.

Sign in to view

HBASE-28883 Manage hbase-thirdparty transitive dependencies via BOM pom

0f3dae4

NihalJain force-pushed the HBASE-28883 branch from 546de8c to 0f3dae4 Compare September 25, 2024 16:24

This comment was marked as outdated.

Sign in to view

ndimiduk reviewed Sep 26, 2024

View reviewed changes

- Remove TODO / demo code

2ce4752

NihalJain marked this pull request as ready for review September 26, 2024 11:26

NihalJain requested a review from ndimiduk September 26, 2024 11:29

ndimiduk approved these changes Oct 14, 2024

View reviewed changes

NihalJain mentioned this pull request Oct 15, 2024

WIP: HBASE-28883 Consume hbase-thirdparty-bom and cleanup impacted depende… apache/hbase#6366

Draft

NihalJain commented Oct 15, 2024

View reviewed changes

+                          </dependency>
+                          <dependency>
+                              <groupId>io.netty</groupId>
+                              <artifactId>netty-bom</artifactId>

+                      <plugins>
+                          <plugin>
+                              <groupId>org.codehaus.mojo</groupId>
+                              <artifactId>flatten-maven-plugin</artifactId>

+                          </dependency>
+                          <dependency>
+                              <groupId>javax.servlet</groupId>
+                              <artifactId>javax.servlet-api</artifactId>

+                  <dependencyManagement>
+                      <dependencies>
+                          <dependency>
+                              <groupId>com.google.protobuf</groupId>

HBASE-28883 Manage hbase-thirdparty transitive dependencies via BOM pom by NihalJain · Pull Request #124 · apache/hbase-thirdparty · GitHub

HBASE-28883 Manage hbase-thirdparty transitive dependencies via BOM pom - #124

HBASE-28883 Manage hbase-thirdparty transitive dependencies via BOM pom#124NihalJain wants to merge 2 commits intoapache:masterapache/hbase-thirdparty:masterfrom NihalJain:HBASE-28883NihalJain/hbase-thirdparty:HBASE-28883Copy head branch name to clipboard

Conversation

NihalJain commented Sep 25, 2024

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

NihalJain Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment was marked as outdated.

ndimiduk Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

NihalJain Sep 26, 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!

ndimiduk Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

NihalJain Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

Apache-HBase commented Sep 26, 2024

Uh oh!

ndimiduk left a comment

Choose a reason for hiding this comment

Uh oh!

ndimiduk commented Oct 14, 2024

Uh oh!

NihalJain commented Oct 15, 2024

Uh oh!

NihalJain Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

NihalJain commented Jan 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HBASE-28883 Manage hbase-thirdparty transitive dependencies via BOM pom#124
NihalJain wants to merge 2 commits into
apache:masterfrom
NihalJain:HBASE-28883

NihalJain commented Sep 25, 2024 •
edited
Loading

NihalJain Sep 26, 2024 •
edited
Loading