apache / polaris

The interoperable, open source catalog for Apache Iceberg
http://polaris.io/
Apache License 2.0
1.02k stars 99 forks source link

Add support for external deps for eclipselink #285

Closed MonkeyCanCode closed 4 days ago

MonkeyCanCode commented 5 days ago

Description

There are couple discussions around how to use eclipselink as well as various issues people ran into after configured the service to use eclispelink (such as missing dependencies for eclipselink as jdbc driver). There is an effort to clear those confusion: https://github.com/apache/polaris/issues/230

However, at the end of the day, people will still need to modify extension/persistence/eclipselink/build.gradle.kts locally and maintain their own fork to be able to use it.

In this PR, I add a small mechanism to allow users to load additional deps they needed to be able to use eclipselink. I am not sure if this is the direction on how we want to move forward with additional driver handling but this should get people unblock and reduce the needed of maintaining a local fork that has the additional dependencies they are using.

I haven't create additional doc for this as I am not sure if that is the direction where we want to move forward. If it is something we would want to support, I will be more than happy to add the needed docs.

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

# Regular build with eclipseLink enabled (no additional dependencies):
## verified no build error
./gradlew --no-daemon --info -PeclipseLink=true clean prepareDockerDist

# build with one additional dependency:
## verified the final jar contains this dependency
./gradlew --no-daemon --info -PeclipseLink=true -PeclipseLinkDeps=org.postgresql:postgresql:42.7.4 clean prepareDockerDist

# build with more than one additional dependencies (separate by comma):
## verified the final jar contains both dependencies
./gradlew --no-daemon --info -PeclipseLink=true -PeclipseLinkDeps=org.postgresql:postgresql:42.7.4,mysql:mysql-connector-java:8.0.33 clean prepareDockerDist

Checklist:

Please delete options that are not relevant.

flyrain commented 5 days ago

I think it's a good idea. Basically, it transfers the responsibility of complying incompatible license to end users. We will have to be careful to not distribute the drivers in any case though. An alternative way is to use a script to wrap the gradle build command as well as to download drivers. But I love the way in this PR better, as it only needs one gradle command. I'm also open to other ideas.

MonkeyCanCode commented 5 days ago

Thanks @MonkeyCanCode for the PR. LGTM overall with a minor comment.

Thanks for the review. Will address those later tonight and update doc to reflect this as well.

MonkeyCanCode commented 4 days ago

LGTM.

Thanks for review. I will add the regex check and update the doc shortly.

MonkeyCanCode commented 4 days ago

Thanks @MonkeyCanCode for the PR. LGTM overall with a minor comment.

Thanks for the review. Will address those later tonight and update doc to reflect this as well.

This had being added and here is the sample output when providing invalid Maven dependencies:

./gradlew --no-daemon --info -PeclipseLink=true -PeclipseLinkDeps=org.postgresql:postgresql,mysql:mysql-connector-java:8.0.33 clean prepareDockerDist

FAILURE: Build failed with an exception.

* Where:
Build file '/home/yong/k8s/polaris/extension/persistence/eclipselink/build.gradle.kts' line: 43

* What went wrong:
Invalid dependency format: org.postgresql:postgresql

===

./gradlew --no-daemon --info -PeclipseLink=true -PeclipseLinkDeps=org.postgresql:postgresql:42.7.4},mysql:mysql-connector-java:8.0.33 clean prepareDockerDist

FAILURE: Build failed with an exception.

* Where:
Build file '/home/yong/k8s/polaris/extension/persistence/eclipselink/build.gradle.kts' line: 43

* What went wrong:
Invalid dependency format: org.postgresql:postgresql:42.7.4}
flyrain commented 4 days ago

Thanks @MonkeyCanCode for working on it. Thanks @aihuaxu for the review.