Skip to content
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

Java11 Prerequisites #4340

Merged
merged 3 commits into from Mar 9, 2022
Merged

Java11 Prerequisites #4340

merged 3 commits into from Mar 9, 2022

Conversation

leifker
Copy link
Contributor

@leifker leifker commented Mar 7, 2022

The key motivation is to update build process and a couple classes for java 11 compatibility.

The default JDK must remain java8 until the spark-lineage module upgrades from Spark 2.x as it has a hard dependency on the java 8 jvm. It is not possible to use the --release flag in later jvms and run spark 2.4 tests.

In order to keep java8 as the default and selectively use java11 we upgrade to gradle 6 with support for toolchains. This allows us to include both java8 and java11 modules, as long as java8 modules do not depend on java11. The reverse is possible, java11 modules can depend on java8 modules.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

@@ -1,6 +1,6 @@
buildscript {
ext.junitJupiterVersion = '5.6.1'
ext.pegasusVersion = '28.3.7'
ext.pegasusVersion = '29.22.16'

Choose a reason for hiding this comment

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

This was bumped to support gradle 6, the old version did not support it. Decided to conservatively increase this version to what appeared to be the latest stable 29.x without moving to the very latest 30.x. Didn't want to move too fast here.

@@ -110,6 +110,9 @@ shadowJar {
relocate 'org.abego.treelayout', 'datahub.shaded.treelayout'
relocate 'org.slf4j', 'datahub.shaded.slf4j'
relocate 'javax.annotation', 'datahub.shaded.javax.annotation'
relocate 'com.github.benmanes.caffeine', 'datahub.shaded.com.github.benmanes.caffeine'

Choose a reason for hiding this comment

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

These were added due to the bump in pegasus version here.

@@ -16,7 +16,7 @@
/**
* In memory ranker that re-ranks results returned by the search backend
*/
public abstract class SearchRanker {
public abstract class SearchRanker<U extends Comparable<? super U>> {

Choose a reason for hiding this comment

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

Compiles with java 11

Copy link
Collaborator

@RyanHolstien RyanHolstien left a comment

Choose a reason for hiding this comment

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

LGTM, changes look pretty straightforward. @shirshanka can we get a forced build execution since this is a first time contribution? Assuming the build goes through I think this is good to go. CI shouldn't need any updates for these prereqs.

@github-actions
Copy link

github-actions bot commented Mar 9, 2022

Unit Test Results (build & test)

  76 files  ±0    76 suites  ±0   15m 45s ⏱️ -30s
629 tests ±0  570 ✔️ ±0  59 💤 ±0  0 ±0 

Results for commit f094a42. ± Comparison against base commit beb51eb.

Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

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

LGTM!

@shirshanka shirshanka merged commit 8bd1db8 into datahub-project:master Mar 9, 2022
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.

None yet

4 participants