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

upgrade dependencies to prepare jdk17 upgrade #563

Merged

Conversation

stevie9868
Copy link
Contributor

@stevie9868 stevie9868 commented Dec 9, 2023

Background:
We are not able to upgrade to jdk17 right now since hms is too old and is not compatible with jdk17. We will deprecate hms soon, and once it is deprecated, metacat can be upgraded to jdk17.
For this PR, we update metacat dependencies besides hms to be compatible with jdk17.

Testing plan:

  1. JDK8 Build pass
  2. Run ./gradlew clean build under jdk17, and build succeed with all unit tests pass.
  3. But ./gradlew functionalTest for jdk17 failed due to hms incompatability issue.

Copy link
Collaborator

@insyncoss insyncoss left a comment

Choose a reason for hiding this comment

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

Could you add a description for why the hms incompatability issue appears and some context around that?

@@ -25,4 +25,8 @@
<Bug code="RCN" />
</Match>

<Match>
<Bug pattern="CT_CONSTRUCTOR_THROW"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good, excluding this is the right way to go

implementation("com.google.inject.extensions:guice-persist")
implementation("com.google.inject.extensions:guice-multibindings")
implementation("com.google.inject.extensions:guice-servlet")
implementation("joda-time:joda-time:2.10.10")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious why this needed to have version pinned to this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't add the version, I will get the following error.

Failed to resolve 'joda-time:joda-time' for project 'metacat-connector-s3'
The following dependencies are missing a version: joda-time:joda-time

@stevie9868
Copy link
Contributor Author

When we try to run functional test under jdk 17, we get the below exception:
Caused by: javax.jdo.JDOFatalInternalException: The java type java.lang.Integer (jdbc-type="", sql-type="") cant be mapped for this datastore. No mapping is available.

Doing some search online:
https://issues.apache.org/jira/browse/SPARK-27538, hms is has a dataneucleus dependency which is not compatible with jdk17.
Also looks like the process to migrate hms to be compatible jdk11 is still not complete https://issues.apache.org/jira/browse/HIVE-22415

Thus we are not able to upgrade metacat to jdk17 due to hms incompatibility with java version older than 8.

@insyncoss insyncoss self-requested a review December 12, 2023 18:11
Copy link
Collaborator

@insyncoss insyncoss left a comment

Choose a reason for hiding this comment

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

Thanks for adding more context, good work

@stevie9868 stevie9868 merged commit 2c0f438 into master Dec 13, 2023
2 checks passed
@insyncoss insyncoss deleted the yingjianw/upgrade_depedencies_to_prepark_jdk17_upgrade branch February 2, 2024 01:45
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.

2 participants