Skip to content

HBASE-29354 Jetty12 dependencies compiled with JDK17 violate hbase-thirdparty bytecode restrictions #139

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

HBASE-29354 Jetty12 dependencies compiled with JDK17 violate hbase-thirdparty bytecode restrictions #139

wants to merge 1 commit into from
+49 −15

Conversation

NihalJain
Copy link
Contributor

  • Override the maxJdkVersion to 17 in the jetty12 related modules
  • Unrelated: Fix spotless issue for existing changes

@NihalJain NihalJain requested review from Apache9, stoty and ndimiduk May 23, 2025 22:37
NihalJain
NihalJain commented May 23, 2025
View reviewed changes
hbase-shaded-jetty-12-plus-core/pom.xml
</dependency>
<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-xml</artifactId>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unrelated spotless issue, can also fix as another commit

@NihalJain
Copy link
Contributor Author

Ran mvn clean verify -Prelease locally with this change.

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Apache HBase Third-Party Libs 4.1.11-SNAPSHOT:
[INFO]
[INFO] Apache HBase Third-Party Libs ...................... SUCCESS [  4.768 s]
[INFO] Apache HBase Patched and Relocated (Shaded) Protobuf SUCCESS [ 30.952 s]
[INFO] Apache HBase Relocated (Shaded) Netty Libs ......... SUCCESS [ 17.538 s]
[INFO] Apache HBase Relocated (Shaded) netty-tcnative Libs  SUCCESS [  5.415 s]
[INFO] Apache HBase Relocated (Shaded) GSON Libs .......... SUCCESS [  0.845 s]
[INFO] Apache HBase Relocated (Shaded) Third-party Miscellaneous Libs SUCCESS [ 10.829 s]
[INFO] Apache HBase Relocated (Shaded) Jetty Libs ......... SUCCESS [ 21.360 s]
[INFO] Apache HBase Relocated (Shaded) Jetty 12+ Libs: Core SUCCESS [  5.965 s]
[INFO] Apache HBase Relocated (Shaded) Jetty 12+ Libs: EE8  SUCCESS [  5.448 s]
[INFO] Apache HBase Relocated (Shaded) Jersey Libs ........ SUCCESS [ 14.518 s]
[INFO] Apache HBase Relocated (Shaded) jackson-jaxrs-json-provider SUCCESS [  1.448 s]
[INFO] Apache HBase Drop-in noop HTrace replacement ....... SUCCESS [01:41 min]
[INFO] Apache HBase Unsafe Wrapper ........................ SUCCESS [  2.655 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  03:44 min
[INFO] Finished at: 2025-05-23T22:37:59Z
[INFO] ------------------------------------------------------------------------

@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 5m 47s 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 _
+0 🆗 mvndep 0m 15s Maven dependency ordering for branch
+1 💚 mvninstall 0m 46s master passed
+1 💚 compile 0m 12s master passed
+1 💚 javadoc 0m 15s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 5s Maven dependency ordering for patch
+1 💚 mvninstall 0m 15s the patch passed
+1 💚 compile 0m 10s the patch passed
+1 💚 javac 0m 10s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 xml 0m 2s The patch has no ill-formed XML file.
+1 💚 javadoc 0m 10s the patch passed
_ Other Tests _
+1 💚 unit 0m 5s hbase-shaded-jetty-12-plus-core in the patch passed.
+1 💚 unit 0m 5s hbase-shaded-jetty-12-plus-ee8 in the patch passed.
+1 💚 asflicense 0m 10s The patch does not generate ASF License warnings.
8m 51s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-Thirdparty-PreCommit/job/PR-139/2/artifact/yetus-precommit-check/output/Dockerfile
GITHUB PR #139
Optional Tests dupname asflicense javac javadoc unit xml compile
uname Linux 02e449d7c86a 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
git revision master / 223f4c6
Default Java Temurin-1.8.0_452-b09
Test Results https://ci-hbase.apache.org/job/HBase-Thirdparty-PreCommit/job/PR-139/2/testReport/
Max. process+thread count 34 (vs. ulimit of 1000)
modules C: hbase-shaded-jetty-12-plus-core hbase-shaded-jetty-12-plus-ee8 U: .
Console output https://ci-hbase.apache.org/job/HBase-Thirdparty-PreCommit/job/PR-139/2/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.

Apache9
Apache9 reviewed May 26, 2025
View reviewed changes
hbase-shaded-jetty-12-plus-core/pom.xml
@@ -218,6 +218,23 @@
</execution>
</executions>
</plugin>
<!-- This module relocates Jetty 12+ EE8 dependencies compiled with JDK 17, hence, overriding the project-level
restriction on maxJdkVersion of 'compileSource'! See HBASE-29354-->
<plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

So here we do not need to compile this, just relocate the pre compiled .class file, which means we do not need to compile this module with JDK17 right?
We used to relocate some jackson classes for JDK9+ too, strange that why there is no problem for it in the past...

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 we do not compile with jdk 17 here. I had even tested out most flows on master even without this change, and things looked good.

Given we wont use these modules for branch 2, shouldnt we be fine with this?

If you think this is not an acceptable approach and we need to think deeper on this, should I drop jetty 12 modules from our main pom temporarily? Or i could rever the jetty change also.

We need release for branch 2.x. This is blocking the upcoming release!

Copy link
Contributor

Choose a reason for hiding this comment

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

We could revert the jetty changes first, make thirdparty 4.1.11 a regular release so we can make the 2.6.x and 2.5.x release, and then back to the jetty 12 related things for 3.x.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure sounds good to me to unblock, let me revert jetty commit, we can come back here once done with 2.x release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better also mention this on the discussion thread on mailing list.

Copy link
Contributor Author

@NihalJain NihalJain May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes sure let me put up over there. Also, I have raised PR to revert at #140

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better also mention this on the discussion thread on mailing list.

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Apache9 Apache9 Apache9 left review comments

@stoty stoty Awaiting requested review from stoty

@ndimiduk ndimiduk Awaiting requested review from ndimiduk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NihalJain @Apache-HBase @Apache9