Isolate Dropwizard and HK2 dependencies #523
Conversation
adutra
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike and
ebyhr
as code owners
adutra
force-pushed
the
polaris-service-dropwizard
branch
3 times, most recently
from
98f8dc8 to
73dd313
Compare
There was a problem hiding this comment.
Big, but "boring" PR ;)
Generally LGTM!
Just would like to have the dropwizard specific stuff in its own top-level directory outside of service:
$POLARIS_ROOT/
dropwizard/service/build.gradle.kts
service/common/build.gradle.kts
This allows us to have modules per concern for the frameworks and for the services. I think that keeps things cleaner in the long run.
gradle/projects.main.properties
Outdated
| @@ -19,7 +19,8 @@ | |||
| # | |||
|
|
|||
| polaris-core=polaris-core | |||
| polaris-service=polaris-service | |||
| polaris-service-common=polaris-service/common | |||
There was a problem hiding this comment.
My thought on "common":
Generally I'm not a fan of any "common module" (it's in almost all projects I know a sink hole for all kinds of different things). I'd be in favor of refactoring this and split it out into separate modules by concern.
But granted that it's currently very hard to split things by concern, we have to do it later.
adutra
force-pushed
the
polaris-service-dropwizard
branch
from
73dd313 to
91766e7
Compare
| val startScripts = | ||
| tasks.named<CreateStartScripts>("startScripts") { | ||
| classpath = files(provider { shadowJar.get().archiveFileName }) | ||
| applicationName = "polaris-service" |
There was a problem hiding this comment.
This is required to keep the script name to polaris-service and the environment variable to $POLARIS_SERVICE_OPTS
There was a problem hiding this comment.
Do we want to keep it as polaris-service or change it to polaris-dropwizard-service as well?
IMO the latter is likely better to not confuse people w/ Quarkus later.
| val startScripts = | ||
| tasks.named<CreateStartScripts>("startScripts") { | ||
| classpath = files(provider { shadowJar.get().archiveFileName }) | ||
| applicationName = "polaris-service" |
There was a problem hiding this comment.
Do we want to keep it as polaris-service or change it to polaris-dropwizard-service as well?
IMO the latter is likely better to not confuse people w/ Quarkus later.
service/common/src/main/java/org/apache/polaris/service/auth/Authenticator.java
Show resolved
Hide resolved
| @@ -108,5 +108,5 @@ The following shows a sample configuration for integrating Polaris with Postgres | |||
| To build Polaris with the necessary Postgres dependency and start the Polaris service, run the following: | |||
| ```bash | |||
| polaris> ./gradlew --no-daemon --info -PeclipseLink=true -PeclipseLinkDeps=org.postgresql:postgresql:42.7.4 clean shadowJar | |||
| polaris> java -jar polaris-service/build/libs/polaris-service-*.jar server ./polaris-server.yml | |||
| polaris> java -jar dropwizard/service/build/libs/polaris-service-*.jar server ./polaris-server.yml | |||
There was a problem hiding this comment.
Related: should the config file be renamed to polaris-dropwizard-service.yml, too?
|
I was hoping to avoid doing this altogether as I thought we had agreed on moving forward with the Quarkus approach in the very near future. At most, I hope this PR is short-lived so that we can continue to have the service implementation and all of its tests in the same gradle package. |
|
@collado-mike : I think it is valuable to isolate services code from runtime-specific code in any case (Quarkus or DW or both). Integration tests have to depend on the runtime, but could also be reused by downstream project where people want to package additional code into the server (by reusing the services module, yet providing custom "application" code), but make sure the functionality is not broken. |
With apache#523, we forgot to update the root package name in the test folder of polaris-dropwizard-service. It's important to do this update to avoid the "split packages" issue (packages appearing in more than one module). This PR has no functional change, however it does make a few methods public in polaris-service-common, in order for them to remain accessible to the moved tests.
Fixes #521.
This PR simply splits
polaris-servicein two modules:polaris-service-commonandpolaris-service-dropwizard, the former bearing no dependency to Dropwizard.99% of the changes are file moves, the rest being minor cleanup.
The tests were all moved to
polaris-service-dropwizardfor now. We need a way to factorize them, but let's tackle that later as it's a complex task.A Quarkus runtime will be provided shortly. It will require some more adjustments to
polaris-service-common, but again, let's proceed step by step.