Introduce multiple datasources#4812
Conversation
43523ce to
345c62a
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Enables selecting among multiple JDBC datasources at runtime for Polaris relational-jdbc persistence, adds an interceptor to activate the chosen datasource, and updates defaults/docs/tests accordingly (including shipping H2 by default).
Changes:
- Introduce
polaris.persistence.relational.jdbc.datasourceand aConfigSourceInterceptorto togglequarkus.datasource.<name>.active. - Add named datasource defaults (H2 + PostgreSQL) and update Gradle dependencies/licenses to ship H2.
- Update documentation and test profiles/resources to use named datasource configuration.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| site/content/in-dev/unreleased/metastores/relational-jdbc.md | Docs updated to configure named PostgreSQL datasource + new Polaris selector property |
| site/content/in-dev/unreleased/helm-chart/production.md | Helm bootstrap example updated to named PostgreSQL datasource env vars |
| site/content/in-dev/unreleased/helm-chart/persistence.md | Helm pool tuning example updated to named PostgreSQL datasource keys |
| site/content/in-dev/unreleased/helm-chart/dev.md | Helm dev bootstrap example updated to named PostgreSQL datasource env vars |
| site/content/in-dev/unreleased/configuration/config-sections/smallrye-polaris_persistence_relational_jdbc.md | Config docs expanded + new datasource selector documented |
| site/content/in-dev/unreleased/admin-tool.md | Admin tool examples updated to named PostgreSQL datasource env vars |
| runtime/test-common/src/main/java/org/apache/polaris/test/commons/RelationalJdbcProfile.java | Test profile renamed to Postgres-specific profile (class name changed) |
| runtime/test-common/src/main/java/org/apache/polaris/test/commons/PostgresRelationalJdbcLifeCycleManagement.java | Postgres test resource config updated to named datasource + selector property |
| runtime/test-common/src/main/java/org/apache/polaris/test/commons/CockroachRelationalJdbcProfile.java | Cockroach test profile overrides simplified |
| runtime/test-common/src/main/java/org/apache/polaris/test/commons/CockroachRelationalJdbcLifeCycleManagement.java | Cockroach test resource config updated to named datasource keys + selector property |
| runtime/service/src/test/java/org/apache/polaris/service/it/PolicyServiceIntegrationTest.java | Integration test profile switched to H2 relational-jdbc profile |
| runtime/service/src/test/java/org/apache/polaris/service/events/listeners/inmemory/InMemoryBufferEventListenerTestBase.java | Tests updated to inject a named datasource |
| runtime/service/src/test/java/org/apache/polaris/service/events/listeners/inmemory/InMemoryBufferEventListenerIntegrationTest.java | Tests updated to inject a named datasource |
| runtime/service/src/test/java/org/apache/polaris/service/Profiles.java | Centralize H2 relational-jdbc test config and apply to multiple profiles |
| runtime/service/src/intTest/java/org/apache/polaris/service/it/relational/jdbc/JdbcViewFileIT.java | ITs updated to use Postgres-specific JDBC profile |
| runtime/service/src/intTest/java/org/apache/polaris/service/it/relational/jdbc/JdbcRestCatalogIT.java | ITs updated to use Postgres-specific JDBC profile |
| runtime/service/src/intTest/java/org/apache/polaris/service/it/relational/jdbc/JdbcPolicyServiceIT.java | ITs updated to use Postgres-specific JDBC profile |
| runtime/service/src/intTest/java/org/apache/polaris/service/it/relational/jdbc/JdbcManagementServiceIT.java | ITs updated to use Postgres-specific JDBC profile |
| runtime/service/src/intTest/java/org/apache/polaris/service/it/relational/jdbc/JdbcApplicationIT.java | ITs updated to use Postgres-specific JDBC profile |
| runtime/service/build.gradle.kts | Add H2 runtime/test dependency (excluding jts-core) |
| runtime/server/distribution/LICENSE | Add H2 driver licensing notice |
| runtime/server/build.gradle.kts | Ship H2 JDBC runtime dependency in server distribution |
| runtime/defaults/src/main/resources/application.properties | Define named datasources (h2/postgresql) + new selector property default |
| runtime/defaults/src/main/resources/application-test.properties | Test defaults updated for datasource activation + disable devservices |
| runtime/defaults/src/main/resources/application-it.properties | IT defaults updated + note about build-time properties |
| runtime/common/src/main/resources/META-INF/services/io.smallrye.config.ConfigSourceInterceptor | Register DataSourceActivator interceptor |
| runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/QuarkusRelationalJdbcConfiguration.java | Add dataSource() to config mapping + richer javadoc |
| runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/JdbcProducers.java | New producer selecting the configured named datasource |
| runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/DataSourceActivator.java | New interceptor toggling datasource .active based on Polaris config |
| runtime/admin/src/testFixtures/java/org/apache/polaris/admintool/AdminProfiles.java | Remove datasource activation overrides from admin tool test profiles |
| runtime/admin/src/main/resources/application.properties | Admin defaults updated to named PostgreSQL datasource + selector property |
| runtime/admin/build.gradle.kts | Align JDBC driver dependencies with Quarkus named datasource approach |
| persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStorePostgresIT.java | Update test config to implement new dataSource() method |
| persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/SimpleRelationalJdbcConfiguration.java | Update test config to implement new dataSource() method |
| persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcProductionReadinessChecksTest.java | Update readiness check test to point at new offending property |
| persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/MetricsReportPersistenceTest.java | Update test config to implement new dataSource() method |
| persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcGrantRecordsIdempotencyTest.java | Update test config to implement new dataSource() method |
| persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcProductionReadinessChecks.java | Update readiness check offending property to new selector property |
| persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcConfiguration.java | Add required dataSource() to persistence config interface |
| persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java | Remove old datasource producer (moved to runtime/common) |
Comments suppressed due to low confidence (1)
site/content/in-dev/unreleased/metastores/relational-jdbc.md:1
- The property key
quarkus.datasource.postgresql.jdbc.jdbc-urlis inconsistent with the rest of this doc (which uses.jdbc.url) and with Quarkus datasource configuration keys (...jdbc.url). Update this entry toquarkus.datasource.postgresql.jdbc.urlto avoid directing users to a non-functional configuration.
bddc63d to
03f46e8
Compare
|
|
||
| @Produces | ||
| @ApplicationScoped | ||
| static DatasourceOperations produceDatasourceOperations( |
There was a problem hiding this comment.
Moved to polaris-runtime-common.
There was a problem hiding this comment.
This is probably for my information, why do we move it? Should this be part of JDBC module for better modularization?
There was a problem hiding this comment.
I had a strong requirement for this move: the new producer method now needs to access io.quarkus.agroal.DataSource.DataSourceLiteral. I noticed that #3960 did the same move for similar reasons.
| Optional<String> databaseType(); | ||
|
|
||
| /** The datasource name to use. Required. */ | ||
| String dataSource(); |
There was a problem hiding this comment.
Note: the datasource name is distinct from the database type.
| # Named datasources for relational persistence backends. | ||
| # Polaris ships with many built-in datasources; by using named datasources, users can configure | ||
| # at runtime which datasources to use for relational persistence backends. | ||
| quarkus.datasource.postgresql.db-kind=postgresql |
There was a problem hiding this comment.
The admin module does not ship H2, at least for now.
We may want to include H2 later. There are pros and cons:
- con: running the admin tool on an ephemeral database is meaningless
- pro: running the admin tool on an ephemeral database allows users to play with the different commands without having to configure a database.
There was a problem hiding this comment.
Not sure H2 helps - at least not as long it's only in-memory.
| public interface QuarkusRelationalJdbcConfiguration extends RelationalJdbcConfiguration {} | ||
| public interface QuarkusRelationalJdbcConfiguration extends RelationalJdbcConfiguration { | ||
|
|
||
| /** The maximum number of retries before giving up the operation. */ |
There was a problem hiding this comment.
We need to repeat the javadocs here for them to appear in the docs.
There was a problem hiding this comment.
Maybe follow-up work in the docs tooling.
| * Maven group:artifact IDs: com.h2database:h2 | ||
|
|
||
| Project URL: https://www.h2database.com/ | ||
| License: Eclipse Public License 1.0 - https://www.eclipse.org/org/documents/epl-1.0/ |
There was a problem hiding this comment.
Slight correction: H2 is dual-licensed: EPL 1.0 and MPL 2.0 - both are category "B" and fine to be included in a binary convenience artifact.
There was a problem hiding this comment.
🤦♂️ - sorry about the confusion.
| runtimeOnly(project(":polaris-relational-jdbc")) | ||
| runtimeOnly("io.quarkus:quarkus-jdbc-postgresql") | ||
| runtimeOnly("io.quarkus:quarkus-jdbc-h2") { | ||
| exclude(group = "org.locationtech.jts", module = "jts-core") |
There was a problem hiding this comment.
This dependency is for geometry types; we don't need it.
| Map.entry("quarkus.datasource.username", postgres.getUsername()), | ||
| Map.entry("quarkus.datasource.password", postgres.getPassword()), | ||
| Map.entry("quarkus.datasource.jdbc.initial-size", "10"), | ||
| // Configure metrics named datasource to use the same PostgreSQL instance |
There was a problem hiding this comment.
394da9c to
8b924f4
Compare
| if (value == null | ||
| || value.getValue() == null | ||
| || active != Boolean.parseBoolean(value.getValue())) { | ||
| value = newConfigValue(value, Boolean.toString(active)); |
There was a problem hiding this comment.
nit: why not do this unconditionally?
There was a problem hiding this comment.
To avoid tampering with the configuration if not absolutely necessary.
There was a problem hiding this comment.
Yes, but it defers any potential bug discovery 🤔
There was a problem hiding this comment.
Do you think so? I can't really imagine a situation where this would be concerning.
There was a problem hiding this comment.
it's a "nit" comment, not a big deal 🙂
| @Inject Instance<DataSource> dataSource; | ||
| @Inject | ||
| @DataSource("h2") | ||
| Instance<AgroalDataSource> dataSource; |
There was a problem hiding this comment.
just wondering: why do we need to change the type here?
There was a problem hiding this comment.
Heh, we don't, but if you keep DataSource, there is an annoying clash between java.sql.DataSource and io.quarkus.agroal.DataSource.
| ## [Unreleased] | ||
|
|
||
| ### Highlights | ||
| - Polaris now supports dynamic datasource activation. Out of the box, two datasources are provided: `postgresql` and `h2`. The datasource to use can be selected at runtime by setting the `polaris.persistence.relational.jdbc.datasource` configuration property (the default is `postgresql`). This allows operators to switch between supported relational databases without rebuilding Polaris. |
There was a problem hiding this comment.
Helm-based deployments are not affected by this change, right?
However, the Admin Tool and plain Polaris Server binaries are affected, I guess 🤔 Should we call this out as a "breaking change"?.. or at least add an "upgrade notice"?
There was a problem hiding this comment.
Hmm actually the Helm chart is affected – but the admin tool isn't (H2 is not available there). Let me rephrase this sentence.
Re: breaking change: the inclusion of a new artifact is not a breaking change, is it? Imho this PR is not a breaking change.
There was a problem hiding this comment.
By "breaking" I mean old quarkus.* config properties that users might have in their scripts/deployment descriptors.
There was a problem hiding this comment.
But I agree that an upgrade notice could be welcome, let me add that.
There was a problem hiding this comment.
Hmm OK, in fact I agree that the configuration changes constitute a breaking change.
| - Names containing control (invisible) characters | ||
| - Names with leading or trailing whitespace | ||
| - Names containing any of these characters: <code>/\:*?"<>|#+`</code> | ||
| - Due to the introduction of dynamic datasource activation, the default (PostgreSQL) datasource is now unused. If you had any custom configuration for that datasource, it should be migrated from `quarkus.datasource.*` to `quarkus.datasource.postgresql.*`. The same is valid for environment variables: `QUARKUS_DATASOURCE_*` should be replaced with `QUARKUS_DATASOURCE_POSTGRESQL_*`. |
There was a problem hiding this comment.
This is a reasonable notice to users, IMHO... However, given that the 1.6.0 release is supposed to happen pretty soon, should we defer merging this PR until after 1.6.0 is branched?
There was a problem hiding this comment.
Yes, that's reasonable. We can defer this to 1.7.0, which would then include the complete switch to JDBC+H2.
There was a problem hiding this comment.
Now that 1.6 is branches, I guess this is no longer a concern ;)
This change enables multiple datasources in Polaris, with possibility of **runtime** activation of the desired datasource. This is achieved by: 1) Introducing a new enabler property: `polaris.persistence.relational.jdbc.datasource`, which points to a named datasource to activate. 2) Declaring, **at build time**, one named datasource for each JDBC driver shipped with the application. 3) Introducing a new `DataSourceActivator` config interceptor that will, at runtime, change the `active` setting of the target datasource to `true`, and all others to `false` (`active` is a runtime setting, contrary to `db-kind`). This change also makes the server ship with the H2 driver by default, since that's the pre-condition for an H2 datasource to be selected at runtime. This change will further enable **as future improvements**: - Using to JDBC + H2 by default for server images, while still allowing users to switch to PostgreSQL *without having to rebuild Polaris*. - Enabling the persistence to "plug" into not one, but many datasources, e.g. a specific datasource for main persistence, and a separate datasource for metrics. This was outlined in apache#3960 – but the machinery to make that happen was missing.
7156f77 to
2fab4d2
Compare
| public interface QuarkusRelationalJdbcConfiguration extends RelationalJdbcConfiguration {} | ||
| public interface QuarkusRelationalJdbcConfiguration extends RelationalJdbcConfiguration { | ||
|
|
||
| /** The maximum number of retries before giving up the operation. */ |
There was a problem hiding this comment.
Maybe follow-up work in the docs tooling.
| # Named datasources for relational persistence backends. | ||
| # Polaris ships with many built-in datasources; by using named datasources, users can configure | ||
| # at runtime which datasources to use for relational persistence backends. | ||
| quarkus.datasource.postgresql.db-kind=postgresql |
There was a problem hiding this comment.
Not sure H2 helps - at least not as long it's only in-memory.
| * Maven group:artifact IDs: com.h2database:h2 | ||
|
|
||
| Project URL: https://www.h2database.com/ | ||
| License: Eclipse Public License 1.0 - https://www.eclipse.org/org/documents/epl-1.0/ |
There was a problem hiding this comment.
Slight correction: H2 is dual-licensed: EPL 1.0 and MPL 2.0 - both are category "B" and fine to be included in a binary convenience artifact.
| quarkus.datasource.postgresql.jdbc.username=<your-username> | ||
| quarkus.datasource.postgresql.jdbc.password=<your-password> | ||
| quarkus.datasource.postgresql.jdbc.jdbc-url=<jdbc-url-of-postgres> |
There was a problem hiding this comment.
| quarkus.datasource.postgresql.jdbc.username=<your-username> | |
| quarkus.datasource.postgresql.jdbc.password=<your-password> | |
| quarkus.datasource.postgresql.jdbc.jdbc-url=<jdbc-url-of-postgres> | |
| quarkus.datasource.postgresql.username | |
| quarkus.datasource.postgresql.password | |
| quarkus.datasource.postgresql.jdbc.url |
| - Names containing control (invisible) characters | ||
| - Names with leading or trailing whitespace | ||
| - Names containing any of these characters: <code>/\:*?"<>|#+`</code> | ||
| - Due to the introduction of dynamic datasource activation, the default (PostgreSQL) datasource is now unused. If you had any custom configuration for that datasource, it should be migrated from `quarkus.datasource.*` to `quarkus.datasource.postgresql.*`. The same is valid for environment variables: `QUARKUS_DATASOURCE_*` should be replaced with `QUARKUS_DATASOURCE_POSTGRESQL_*`. |
There was a problem hiding this comment.
Now that 1.6 is branches, I guess this is no longer a concern ;)
This change enables multiple datasources in Polaris, with possibility of runtime activation of the desired datasource.
This is achieved by:
Introducing a new enabler property:
polaris.persistence.relational.jdbc.datasource, which points to a named datasource to activate.Declaring, at build time, one named datasource for each JDBC driver shipped with the application.
Introducing a new
DataSourceActivatorconfig interceptor that will, at runtime, change theactivesetting of the target datasource totrue, and all others tofalse(activeis a runtime setting, contrary todb-kind).This change also makes the server ship with the H2 driver by default, since that's the pre-condition for an H2 datasource to be selected at runtime.
This change will further enable as future improvements:
Using to JDBC + H2 by default for server images, while still allowing users to switch to PostgreSQL without having to rebuild Polaris.
Enabling the persistence to "plug" into not one, but many datasources, e.g. a specific datasource for main persistence, and a separate datasource for metrics. This was outlined in Introduce DataSourceResolver for multi-datasource support in JDBC per… #3960 – but the machinery to make that happen was missing.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)