-
Notifications
You must be signed in to change notification settings - Fork 980
[BUILD] [GA] Support JDK 11 and enable it in GitHub Action #693
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
Codecov Report
@@ Coverage Diff @@
## master #693 +/- ##
==========================================
- Coverage 80.49% 0.00% -80.50%
==========================================
Files 122 122
Lines 4748 4748
Branches 576 576
==========================================
- Hits 3822 0 -3822
- Misses 605 4748 +4143
+ Partials 321 0 -321
Continue to review full report at Codecov.
|
acf755a to
7e20704
Compare
|
can you fix the conflicts? and open another issue to track the perf regress for jdk11? |
|
Rebased on master and open a new issue #745 to track the performance regression. |
| <artifactId>hive-service-rpc</artifactId> | ||
| </dependency> | ||
|
|
||
| <dependency> |
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.
who/where use this in the common module?
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.
We don't use it. jaxb-api should be considered as a transitive dep of hadoop-client-runtime.
If we move jaxb-api to test scope, any accident invocation of Hadoop API which direct or indirect requires jaxb-api, the ut pass, but will cause runtime failures in production.
kyuubi-main/src/test/scala/org/apache/kyuubi/operation/KyuubiOperationPerConnectionSuite.scala
Show resolved
Hide resolved
kyuubi-main/src/test/scala/org/apache/kyuubi/operation/KyuubiOperationYarnClusterSuite.scala
Show resolved
Hide resolved
pom.xml
Outdated
| <reportsDirectory>${project.build.directory}/surefire-reports</reportsDirectory> | ||
| <junitxml>.</junitxml> | ||
| <filereports>TestSuite.txt</filereports> | ||
| <argLine>-Xmx4g -Xss4m -XX:ReservedCodeCacheSize=512M</argLine> |
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.
-Xmx4g -Xms4g -Xss32m -XX:ReservedCodeCacheSize=512M
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.
After changes, GA failed again, now shrink to -Xmx2g -Xms2g -Xss8m -XX:ReservedCodeCacheSize=512M and waiting GA results.
|
|
||
| <profile> | ||
| <id>java-8</id> | ||
| <activation> |
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.
what is this for?
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.
auto activate profile by jdk version
|
Thanks for reviewing, as all comments are addressed and GA passed, I'm going to merge it for v1.3.0. |
|
late lgtm |
Why are the changes needed?
Close #550, relate to #559 #688 #689.
Add
javax.xml.bind:jaxb-apiandjavax.activation:activationsince those apis has been removed by JDK 11.In GA, we saw a significant performance downgrade in the unit tests, and even with some changes like enlarging timeout, using in-memory catalog instead of hive catalog, the JDK11(~43min) tests cost 2 times than JDK8(~22min), this performance regression is tracked in #745.
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request