Skip to content

Conversation

@sandeep318kumar
Copy link
Contributor

@sandeep318kumar sandeep318kumar commented Sep 23, 2024

[AMBARI-26142] JDK17 support for Ambari
Co-authored-by: Mohammad Arshad [email protected]

What changes were proposed in this pull request?

Changed java version in pom from 1.8 to 17
Changed maven-shade-plugin version to 3.5.1 to fix jar shading related issue
Changed phonix version to 5.1.3.3.2.2.4-13 so correct hadoop dependency jar is used.
Changed maven-failsafe-plugin to 3.2.5 to fix build issue
Changed eclipselink version from 2.6.2 to 2.7.14
Changed guice version from 4.1.0 to 5.1.0
Made changes to adapt new API
Made change to handle API behavior change
Using latest power mock 2.0.9 and corresponding, mockito 2.28.2 and easymock 5.2.0
Removed Unnecessary Subbing which is not allowed now.
Corrected reflection usages
Fixed Duplicate column name UPGRADE_ID; SQL statement
Corrected few test cases, rewritten PersistServiceTest

(Please fill in changes proposed in this fix)

How was this patch tested?

TESTING is pending

(Please explain how this patch was tested. Ex: unit tests, manual tests)
(If this patch involves UI changes, please attach a screen-shot; otherwise, remove this)

Please review Ambari Contributing Guide before opening a pull request.

@JiaLiangC
Copy link
Contributor

@sandeep318kumar Excellent work. Since upgrading to JDK 17 is a major change, it might be reasonable to split this into multiple PRs.

@sandeep318kumar sandeep318kumar changed the title JDK17 support for Ambari [AMBARI-26040] JDK17 support for Ambari Sep 24, 2024
@sandeep318kumar
Copy link
Contributor Author

@sandeep318kumar Excellent work. Since upgrading to JDK 17 is a major change, it might be reasonable to split this into multiple PRs.

There are multiple build failures, we need to resolve all build failures and test failures that's why keeping everything in one PR.

@sandeep318kumar sandeep318kumar changed the title [AMBARI-26040] JDK17 support for Ambari [AMBARI-26142] JDK17 support for Ambari Sep 24, 2024
@sandeep318kumar
Copy link
Contributor Author

Screenshot 2024-09-25 at 10 43 27 AM

we're setting JDK1.8 here, How can we setup JDK17 in CI?

@JiaLiangC
Copy link
Contributor

Screenshot 2024-09-25 at 10 43 27 AM we're setting JDK1.8 here, How can we setup JDK17 in CI?

Regarding the issue of switching Java versions in CI/CD for Ambari, I will try to resolve it and seek support from the Apache infra team. In the meantime, you can run the tests locally.

@sandeep318kumar
Copy link
Contributor Author

Tests are passing locally.


Ran 296 tests in 13.655s

OK

Total run:352
Total errors:0
Total failures:0
OK
[INFO]
[INFO] --- maven-checkstyle-plugin:2.17:check (checkstyle) @ ambari-server ---
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Ambari Main 3.0.0.0-SNAPSHOT:
[INFO]
[INFO] Ambari Main ........................................ SUCCESS [ 12.061 s]
[INFO] Apache Ambari Project POM .......................... SUCCESS [ 0.074 s]
[INFO] Ambari Views ....................................... SUCCESS [ 5.185 s]
[INFO] ambari-utility ..................................... SUCCESS [ 11.138 s]
[INFO] Ambari Server SPI .................................. SUCCESS [ 0.787 s]
[INFO] Ambari Service Advisor ............................. SUCCESS [ 0.775 s]
[INFO] Ambari Server ...................................... SUCCESS [03:12 min]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 03:48 min
[INFO] Finished at: 2024-09-26T02:05:53+05:30
[INFO] ------------------------------------------------------------------------

@JiaLiangC
Copy link
Contributor

@sandeep318kumar I have created an infra issue, and now we just need to wait for the infra team’s support and resolution. You can also check the issue to track its progress. https://issues.apache.org/jira/browse/INFRA-26157

@sandeep318kumar
Copy link
Contributor Author

sandeep318kumar commented Oct 4, 2024

@JiaLiangC @virajjasani
I have updated the JDK in jenkinsfile, but still jdk1.8 is being set up in CI. Can you see how to have JDK17?

@JiaLiangC
Copy link
Contributor

@JiaLiangC @virajjasani I have updated the JDK in jenkinsfile, but still jdk1.8 is being set up in CI. Can you see how to have JDK17?

because only committer or pmc member can update jenkinsfile, i will temporarily change the permission.

@JiaLiangC
Copy link
Contributor

image
image
@sandeep318kumar Hello, I have modified the Jenkins permissions, and you can now use the Jenkins file submitted in the current PR. As shown in the image, JDK 17 is being used, but there are still many compilation errors. Please address them.

@sandeep318kumar
Copy link
Contributor Author

@JiaLiangC Thanks!
I'll resolve all failures/errors.

@sandeep318kumar sandeep318kumar force-pushed the JDK17 branch 2 times, most recently from 3601572 to c16f7cd Compare October 8, 2024 14:58
@JiaLiangC
Copy link
Contributor

@sandeep318kumar Hello,
Upgrading the JDK is a significant change, including upgrading Spring. Can we list a series of subtasks, then create a new branch to merge these changes into first? Once it's stable, we can merge it into the trunk branch. This approach will also make it easier for more people to get involved.

@JiaLiangC
Copy link
Contributor

@sandeep318kumar Hello, Upgrading the JDK is a significant change, including upgrading Spring. Can we list a series of subtasks, then create a new branch to merge these changes into first? Once it's stable, we can merge it into the trunk branch. This approach will also make it easier for more people to get involved.

@virajjasani Any idea on your side?

@sandeep318kumar
Copy link
Contributor Author

@JiaLiangC
spring upgrade we can do after JDK17 only.
In this PR, I have made the code changes. There are several Java test case failures. I'm working on resolving them.
Do you want me to create a separate PR for that?

@JiaLiangC
Copy link
Contributor

@sandeep318kumar I mean: Upgrade JDK to 17 and Spring to 6, both should be merged into a new branch. Once the new branch is thoroughly tested, it can be merged into master. Previously, I merged a large PR for the frontend jquery upgrade directly into master, which caused many bugs in the ambari frontend. Therefore, for significant changes involving APIs, it's recommended to first merge into a new branch, test thoroughly, and then merge into master. Spring indeed needs to wait for JDK 17 to be completed first.

@JiaLiangC
Copy link
Contributor

If there are no objections, I will create a new branch specifically for handling JDK and Spring upgrades, as well as other related dependency upgrade PRs

@virajjasani
Copy link
Contributor

The idea of working on a new branch seems good. @sandeep318kumar Thanks for the patch, we can resume the work on the feature branch?

@sandeep318kumar
Copy link
Contributor Author

@JiaLiangC @virajjasani New feature branch is good for working on this. Can you create a new feature branch for this.

@JiaLiangC
Copy link
Contributor

@sandeep318kumar I've created a new branch named upgrade/jdk-spring-dependencies for handling JDK, Spring, and other related dependency upgrades. We can use this branch for any relevant updates and pull requests.

@JiaLiangC
Copy link
Contributor

@sandeep318kumar Thank you for your contribution. I ran the unit tests and found several failing tests, as well as some tests that hang. It seems like fixing all of these issues will be a significant amount of work. Could you please submit your code to the upgrade/jdk-spring-dependencies branch? I'll merge it, and then the community can collaborate to fix the remaining tests.

@sandeep318kumar
Copy link
Contributor Author

Sure @JiaLiangC,
Thanks for creating the new feature branch.
I'll raise JDK17 upgrade PR there.

@JiaLiangC
Copy link
Contributor

@sandeep318kumar I will close this PR because it is a duplicate of #3851, which has already been merged.

@sandeep318kumar
Copy link
Contributor Author

Closing this PR as the duplicate PR #3851 has been already merged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants