-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-15984. Update jersey from 1.19 to 2.x. #7019
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
The code for |
3b35b60 to
820dc89
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
I am continuing to follow up on the remaining issues, including sputbug, checkstyle, |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
7323c92 to
6971140
Compare
|
💔 -1 overall
This message was automatically generated. |
|
Most of the issues have been resolved, but the overall renovation time has taken longer than expected. There are still some problems with checkstyle, spotbugs, and shadeclient that are being addressed. I hope to use the weekend to resolve these issues. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
0e787f9 to
eb52efc
Compare
|
💔 -1 overall
This message was automatically generated. |
steveloughran
left a comment
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.
+1
I think we can merge to trunk and stabilise. I'm trying to build/test hadoop cloud connector modules against java 17, and jersey failures are surfacing there...this will fix that
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@steveloughran @Hexiaoqiao @pjfanning Thank you all for reviewing this PR! We have compiled it multiple times, and currently, the failing unit tests seem unrelated to our changes. Thanks to @pjfanning for the suggestions—they have been really helpful. Based on the current progress, I plan to merge this PR first and then assess if there are any other improvements we can submit in future PRs. Additionally, I will revert the changes to |
|
💔 -1 overall
This message was automatically generated. |
|
Thanks all for your great works. make sense to. +1 from my side. |
|
@Hexiaoqiao @steveloughran Thank you for approving this PR! If any issues arise in the future, I will continue to follow up. However, I remain optimistic since we haven't changed the core logic, and the primary changes are related to testing. I will also keep optimizing the code to prepare for a future upgrade to Jersey 3, and I already have some ideas for that. I plan to merge this PR in 2 working days. I will add the following members as co-authors in the commit to express my gratitude. @aajisaka @@virajjasani These two members provided the initial version of HADOOP-15984. At the same time, I hope our JDK 17 upgrade goes smoothly. After this PR, we plan to submit a large-scale PR to upgrade from JUnit4 to JUnit5. We have already prepared for this locally. |
|
I plan to merge this PR into the trunk branch and monitor the community build emails and test reports from the past two days. If the reports look good, I will backport the changes to branch-3.4 to support JDK 17 compatibility for the hadoop-3.4.2 release. A big thanks again to all the members who have reviewed and provided assistance with this PR. |
|
I believe the overall upgrade is currently in line with expectations. I have reviewed the overall build report, and there are no issues with JDK 8. However, with JDK 11, there is a reported issue regarding the missing |
|
Huge thanks @slfan1989 and all the reviewers. |
@aajisaka Thank you for your message! You helped take the first step in this PR, as the code merged into the community came from your submission, especially the HDFS part, which is crucial. Thanks again to the members who have been following this JIRA. |
|
think the junit changes broke a lot of testing running; for now I'm rebasing my prs back onto the commit before that. I don't want to revert this (it's big!), but I need to understand why junit 4 tests would stop running and what is needed to turn them back on |
|
@steveloughran I am prioritizing the investigation of this issue. If no other solutions are found, we will roll back this PR. I believe the problem might be related to the changes I made to the JUnit dependencies in the POM file. I'll check and try to submit a fix PR. I won't keep everyone waiting for too long. I've paused other tasks and am prioritizing this issue. |
|
wee good to backport this to branch-3.4? would round off the dependency updates |
I don't think such a big change should go in a patch release. Could we not just expedite a 3.5.0 release and push any incomplete 3.5 work to 3.5.1 or 3.6.0? |
@steveloughran @pjfanning From a migration perspective, it's feasible, but I agree with pjfanning's view because this PR is indeed quite large. At the moment, I haven't fully clarified the positioning of branch-3.4. Is it the last branch to support JDK 8? If we ultimately decide to migrate the changes to branch-3.4, I will take care of it. cc: @Hexiaoqiao @ayushtkn |
|
ok. will delgate the internal backport to others and externally we can focus on the next release after 3.4.2 being that 3.5.0 on java17 |
Co-authored-by: Akira Ajisaka <[email protected]> Co-authored-by: Viraj Jasani <[email protected]> Co-authored-by: Ayush Saxena <[email protected]> Co-authored-by: PJ Fanning <[email protected]> Co-authored-by: Steve Loughran <[email protected]> Co-authored-by: He Xiaoqiao <[email protected]> Reviewed-by: Steve Loughran <[email protected]> Reviewed-by: He Xiaoqiao <[email protected]> Signed-off-by: Shilun Fan <[email protected]>
Co-authored-by: Akira Ajisaka <[email protected]> Co-authored-by: Viraj Jasani <[email protected]> Co-authored-by: Ayush Saxena <[email protected]> Co-authored-by: PJ Fanning <[email protected]> Co-authored-by: Steve Loughran <[email protected]> Co-authored-by: He Xiaoqiao <[email protected]> Reviewed-by: Steve Loughran <[email protected]> Reviewed-by: He Xiaoqiao <[email protected]> Signed-off-by: Shilun Fan <[email protected]>
Co-authored-by: Akira Ajisaka <[email protected]> Co-authored-by: Viraj Jasani <[email protected]> Co-authored-by: Ayush Saxena <[email protected]> Co-authored-by: PJ Fanning <[email protected]> Co-authored-by: Steve Loughran <[email protected]> Co-authored-by: He Xiaoqiao <[email protected]> Reviewed-by: Steve Loughran <[email protected]> Reviewed-by: He Xiaoqiao <[email protected]> Signed-off-by: Shilun Fan <[email protected]>
Co-authored-by: Akira Ajisaka <[email protected]> Co-authored-by: Viraj Jasani <[email protected]> Co-authored-by: Ayush Saxena <[email protected]> Co-authored-by: PJ Fanning <[email protected]> Co-authored-by: Steve Loughran <[email protected]> Co-authored-by: He Xiaoqiao <[email protected]> Reviewed-by: Steve Loughran <[email protected]> Reviewed-by: He Xiaoqiao <[email protected]> Signed-off-by: Shilun Fan <[email protected]>
This file breaks using jaxb in the same classpath where Hadoop is present if JVM 9+ See ticket: https://issues.apache.org/jira/browse/HADOOP-17540 This was fixed on apache#7019 for trunk, but the 3.4 branch still has the problem.
Description of PR
JIRA: HADOOP-15984. Update jersey from 1.19 to 2.x.
Many members of the community have tried to help upgrade this component, and there are numerous PRs available for reference. I have also reviewed a lot of the code they submitted. I would like to express my sincere gratitude to them.
Differences from previous approaches
My approach differs from previous upgrade PRs in the following two main ways:
I did not insist on binding
GuiceandJerseytogether. InJersey 1.x, the official Jersey integration with Guice made dependency injection easier. However, inJersey 2.x, this integration has become quite weak and presents many issues during the integration process.I allowed
GuiceandJersey2to manage their own injected objects independently. Based on practical experience, this approach is feasible and does not cause conflicts.Key points of the upgrade include
We are attempting to upgrade Jersey to version 2.x to enable Hadoop to support higher JDK versions in the future. During the upgrade process, the following modifications were made:
For version selection, I chose Jersey
2.46.Jersey 2.x is Java EE / Jakarta EE 8 / JAX-RS 2.1 / Jakarta REST 2.1 compatible.
Jakartapackages to replace the originalJavaxpackages.javax.xml.bind -> jakarta.xml.bind
javax.activation -> jakarta.validation
javax.servlet -> jakarta.servlet-api
1.9.4, including the related exclusion tags in the POM.How was this patch tested?
Junit Test.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?