Skip to content

Move JDBC Quarkus configuration to the JDBC persistence module #3281

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.

Sign up for GitHub

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

Open
flyrain wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Move JDBC Quarkus configuration to the JDBC persistence module #3281

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bfe6530
Remove module polaris-runtime-common
flyrain Dec 16, 2025
715652c
Remove module polaris-runtime-common
flyrain Dec 16, 2025
e4ebacc
Resolve comments
flyrain Dec 16, 2025
9599cff
Resolve comments
flyrain Dec 18, 2025
dc4e343
Fix a hidden bug
flyrain Dec 18, 2025
11a71cb
Fix a hidden bug
flyrain Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion bom/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ dependencies {
api(project(":polaris-extensions-auth-opa"))

api(project(":polaris-admin"))
api(project(":polaris-runtime-common"))
api(project(":polaris-runtime-test-common"))
api(project(":polaris-runtime-defaults"))
api(project(":polaris-server"))
Expand Down
1 change: 0 additions & 1 deletion gradle/projects.main.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ polaris-server=runtime/server
polaris-distribution=runtime/distribution
polaris-runtime-spark-tests=runtime/spark-tests
polaris-admin=runtime/admin
polaris-runtime-common=runtime/common
polaris-runtime-test-common=runtime/test-common
polaris-relational-jdbc=persistence/relational-jdbc
polaris-tests=integration-tests
Expand Down
3 changes: 3 additions & 0 deletions persistence/relational-jdbc/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,11 @@ dependencies {
compileOnly(libs.jakarta.enterprise.cdi.api)
compileOnly(libs.jakarta.inject.api)

compileOnly(libs.smallrye.config.core) // @ConfigMapping for Quarkus integration
implementation(libs.smallrye.common.annotation) // @Identifier
implementation(libs.postgresql)
implementation(platform(libs.quarkus.amazon.services.bom))
Copy link
Contributor

@adutra adutra Dec 16, 2025

Choose a reason for hiding this comment

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

This new dependency is intriguing. This module doesn't need the RDS extension for compilation, not even for tests. If the intent is to create a Polaris server distribution that supports Amazon RDS, I wonder if a better approach wouldn't be to include the RDS Quarkus extension directly in polaris-server?

Conversely, if we decide to keep this dependency here: it is bringing quarkus-core transitively. This means that this module becomes de facto a Quarkus module. In this case we should declare the dependency implementation(platform(libs.quarkus.bom)) and add the Quarkus plugin.

Copy link
Contributor

@adutra adutra Dec 16, 2025

Choose a reason for hiding this comment

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

Oh I see now, the new dependency was added in #2650 to runtime-common, and @dimas-b is already taking a stab at moving this dependency to the appropriate place in #3252.

I suggest removing this dependency from here and use #3252 to find a better place for it. Wdyt?

Copy link
Contributor Author

@flyrain flyrain Dec 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It isn't new dependency. As you said, it was there for a while. It looks like #3252 didn't do anything about it. Let me know if I missed something.
At this moment, I think it'd be nice to keep as is until we find a better place to hold it. I'm a bit concern that if we removed it here, but didn't get the chance to place it anywhere before next release, it will break users. With that, we might just keep this PR as a pure refactor. WDYT?

Copy link
Contributor

@adutra adutra Dec 16, 2025

Choose a reason for hiding this comment

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

I think it was introduced recently by #2650 – at least I don't see any traces of RDS in Polaris before that.

That said, yes, I agree to move the dependency here and keep thinking about a better home for it.

@dimas-b would you be OK with this?

Copy link
Contributor

@dimas-b dimas-b Dec 16, 2025

Choose a reason for hiding this comment

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

I do believe that runtime library dependencies should be concentrated in the runtime/server module.

So, I do not think bringing RDS as an implementation dependency into JDBC persistence is justified. JDBC persistence code should not depend on specific drivers, only on JDBC interfaces.

Copy link
Contributor

@dimas-b dimas-b Dec 18, 2025

Choose a reason for hiding this comment

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

JDBC Persistence and other similar modules compile against their respective interfaces.

The runtime/server module includes drivers (PostgreSQL, RDS, etc.) according to the Polaris project decisions - whatever is supposed to be included in the ASF releases. This does not have to be an all-encompassing list.

In this regard, the runtime/server module is essentially the "default" server "package".

Downstream projects are not supposed to reuse runtime/server directly.

Downstream projects reuse JDBC Persistence, runtime/service, etc... as needed and build a custom server. Downstream projects also include libraries / drivers as needed according to the needs of the downstream project.

For example, PostgreSQL or RDS code may or may not be included - the decision should rest with the downstream project IMHO. A downstream project may even choose to use a completely different JDBC driver.

Conversely, with the current state of this PR, RDS is an impl. dependency of JDBC Persistence and NOT including it in downstream builds requires extra work at explicitly preventing RDS jars from leaking into custom builds... which is very inconvenient.

Copy link
Contributor Author

@flyrain flyrain Dec 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the explanation.
The default dependencies, esp. drivers are not only used by runtime/server, but also the admin module. Instead of adding them to both, how about putting them in the runtime/default module, and let both admin and server depend on runtime/default? I think the downstream projects are also not supposed to directly reuse the runtime/default module. WDYT?

Copy link
Contributor

@dimas-b dimas-b Dec 18, 2025

Choose a reason for hiding this comment

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

Good point about adding common server/admin dependencies to runtime/default... I kind of forgot about it 😅 but indeed I added it a long time ago precisely for this purpose 👍

Copy link
Contributor

@dimas-b dimas-b Dec 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

With that, I'm probably not going to need #3252 at all :) ... but I'll revisit after this PR is merged ;)

Copy link
Contributor Author

@flyrain flyrain Dec 18, 2025

Choose a reason for hiding this comment

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

Fixed it in the new commits. I think we should start to put the driver in the default module moving forward. We could move this to the runtime/default as well in a followup.

runtimeOnly("org.postgresql:postgresql")

With that, I'm probably not going to need #3252 at all :)

You can buy me a drink :-)

implementation("io.quarkiverse.amazonservices:quarkus-amazon-rds")

compileOnly(project(":polaris-immutables"))
annotationProcessor(project(":polaris-immutables", configuration = "processor"))
Expand Down
2 changes: 2 additions & 0 deletions ...main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
*/
package org.apache.polaris.persistence.relational.jdbc;

import io.smallrye.config.ConfigMapping;
import java.util.Optional;

@ConfigMapping(prefix = "polaris.persistence.relational.jdbc")
Copy link
Contributor

@dimas-b dimas-b Dec 16, 2025

Choose a reason for hiding this comment

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

I do not oppose this change, but I'd like to highlight that this will impose the config naming convention defined in this annotation on downstream projects. It may be possible to provide a different config object for JDBC downstream or it may not be... I cannot say for sure.

Old code delegated runtime configuration to the server runtime, so this may be a behaviour change in downstream projects.

Given that NoSQL code follows a similar approach (not splitting interfaces and @ConfigMapping) this change looks reasonable to me.

public interface RelationalJdbcConfiguration {
// max retries before giving up
Optional<Integer> maxRetries();
Expand Down
2 changes: 0 additions & 2 deletions runtime/admin/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ dependencies {
implementation("io.quarkus:quarkus-picocli")
implementation("io.quarkus:quarkus-container-image-docker")

implementation(project(":polaris-runtime-common"))

testImplementation(project(":polaris-runtime-test-common"))
testFixturesApi(project(":polaris-core"))

Expand Down
30 changes: 0 additions & 30 deletions runtime/common/build.gradle.kts
View file
Open in desktop

This file was deleted.

25 changes: 0 additions & 25 deletions ...ava/org/apache/polaris/quarkus/common/config/jdbc/QuarkusRelationalJdbcConfiguration.java
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion runtime/service/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ dependencies {
runtimeOnly(project(":polaris-relational-jdbc"))

implementation(project(":polaris-runtime-defaults"))
implementation(project(":polaris-runtime-common"))

compileOnly(project(":polaris-immutables"))
annotationProcessor(project(":polaris-immutables", configuration = "processor"))
Expand Down