Move JDBC Quarkus configuration to the JDBC persistence module #3281

flyrain · 2025-12-16T05:49:34Z

Removes the polaris-runtime-common module and updates dependent modules accordingly.

Quarkus/JDBC configuration is moved to and owned by the JDBC persistence module, where it fits more naturally. No behavior changes expected.

Checklist

🛡️ Don't disclose security issues! (contact [email protected])
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed)
📚 Updated documentation in site/content/in-dev/unreleased (if needed)

adutra · 2025-12-16T13:23:10Z

.../java/org/apache/polaris/persistence/relational/jdbc/QuarkusRelationalJdbcConfiguration.java

-import org.apache.polaris.persistence.relational.jdbc.RelationalJdbcConfiguration;

 @ConfigMapping(prefix = "polaris.persistence.relational.jdbc")
 public interface QuarkusRelationalJdbcConfiguration extends RelationalJdbcConfiguration {}


This subinterface is now useless. I suggest removing it and placing the @ConfigMapping annotation directly on RelationalJdbcConfiguration.

Great idea! Changed per your suggestion.

adutra · 2025-12-16T13:27:18Z

persistence/relational-jdbc/build.gradle.kts

+  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))


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.

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?

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?

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?

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.

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.

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?

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 👍

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

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 :-)

singhpk234

LGTM, thanks @flyrain

dimas-b · 2025-12-16T20:09:44Z

...rc/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcConfiguration.java

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

+@ConfigMapping(prefix = "polaris.persistence.relational.jdbc")


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.

dimas-b · 2025-12-16T20:12:40Z

persistence/relational-jdbc/build.gradle.kts

+  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))


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.

flyrain · 2025-12-18T02:45:47Z

...rc/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java

+        metaStoreManagerMap.remove(realm);
+      } catch (IllegalStateException e) {
+        // Realm is not bootstrapped, return a failed result
+        results.put(realm, new BaseResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, e.getMessage()));


To fix

RelationalJdbcPurgeCommandTest > testPurgeFailure(LaunchResult) FAILED org.assertj.core.error.AssertJMultipleFailuresError at PurgeCommandTestBase.java:48

This is needed as runtime-defaults has configuration file application.properties with Quarkus configuration like logging, console, or output-related settings, which interferes with how Quarkus Main tests capture stdout. When the exception is thrown, the output isn't captured in LaunchResult.getOutput(). It only goes to logs. This causes the test assertion to fail because result.getOutput() is empty.

The fix ensures that when a realm isn't bootstrapped, the PurgeCommand receives a failed BaseResult and can properly write the error message to stdout,

polaris/runtime/admin/src/main/java/org/apache/polaris/admintool/PurgeCommand.java

Line 53 in dc4e343

failed.forEach(

, instead of catching an exception and only writing to stderr (line 62).

Hi @flyrain , the logic in your message LGTM, but the logic in the code is not specific enough, I'm afraid.

Catching IllegalStateException does not necessarily mean that the failure is due to realm not having been bootstrapped.

Is it possible to make this change more specific to the bootstrap use case?

I'm fine with catching IllegalStateException in general, but why should the status be ENTITY_NOT_FOUND when we do not really know the root reason 🤔 ?

Actually, the matter might be related to mixing runtime/admin/src/main/resources/application.properties and runtime/defaults/src/main/resources/application.properties (or perhaps application-test.properties).

In my experience having more than one application.properties file in a Quarkus build leads to very obscure behaviours. So far I thought it was only happening when external jars (with application.properties were on the class path), but apparently the oddities happen in a multi-module build too 🤔

I'm afraid the runtime/defaults module will have to be limited to holding only the application.properties for servers (shared with test modules).

We can probably keep the Quarkus config refactoring in this PR, but the common module may have to stay to hold dependencies shared between the server and admin tools. WDYT?

flyrain added 2 commits December 15, 2025 21:42

Remove module polaris-runtime-common

bfe6530

Remove module polaris-runtime-common

715652c

github-project-automation bot added this to Basic Kanban Board Dec 16, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Dec 16, 2025

flyrain requested review from adutra and singhpk234 December 16, 2025 05:49

flyrain changed the title ~~Remove runtime common~~ Move JDBC Quarkus configuration to the JDBC persistence module Dec 16, 2025

flyrain closed this Dec 16, 2025

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Dec 16, 2025

flyrain reopened this Dec 16, 2025

github-project-automation bot moved this from Done to PRs In Progress in Basic Kanban Board Dec 16, 2025

adutra reviewed Dec 16, 2025

View reviewed changes

Resolve comments

e4ebacc

flyrain closed this Dec 16, 2025

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Dec 16, 2025

flyrain reopened this Dec 16, 2025

github-project-automation bot moved this from Done to PRs In Progress in Basic Kanban Board Dec 16, 2025

singhpk234 previously approved these changes Dec 16, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Dec 16, 2025

dimas-b reviewed Dec 16, 2025

View reviewed changes

Resolve comments

9599cff

flyrain dismissed singhpk234’s stale review via 9599cff December 18, 2025 01:51

Fix a hidden bug

dc4e343

flyrain commented Dec 18, 2025

View reviewed changes

Fix a hidden bug

11a71cb

Move JDBC Quarkus configuration to the JDBC persistence module #3281

Are you sure you want to change the base?

Move JDBC Quarkus configuration to the JDBC persistence module #3281

Uh oh!

Conversation

flyrain commented Dec 16, 2025

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flyrain Dec 16, 2025 •

edited

Loading

flyrain Dec 18, 2025 •

edited

Loading

dimas-b Dec 18, 2025 •

edited

Loading

dimas-b Dec 18, 2025 •

edited

Loading