Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
github-actions[bot] commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4468882246 This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4201658101 @flyrain @singhpk234 : please review the latest changes proposed by @Subham-KRLX . From my POV, the PR is moving in the right direction, but I'd appreciate reviews from both of you before we proceed. Thx! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r3025503617
##
site/content/in-dev/unreleased/proposals/jdbc-multi-datasource.md:
##
@@ -0,0 +1,90 @@
+
+# Design: Multi-DataSource support in JDBC persistence
+
+## Goal
+The goal of this design is to decouple `DataSource` selection from the core
JDBC persistence logic. This allows for:
+1. **Workload Isolation**: Separating the Metastore, Metrics reporting, and
Event logging into different physical databases or connection pools.
+2. **Scalable Multi-Tenancy**: Enabling per-realm (tenant) routing to support
large-scale deployments.
+
+## Core Interface: `DataSourceResolver`
+A service interface designed to resolve the correct `DataSource` based on the
workload's metadata.
+
+```java
+public interface DataSourceResolver {
+ enum StoreType {
+METASTORE,
+METRICS,
+EVENTS
+ }
+
+ DataSource resolve(RealmContext realmContext, StoreType storeType);
+}
+```
+
+### Key Components
+- **`DataSourceResolver`**: SPI for resolution logic.
+- **`DefaultDataSourceResolver`**: Backward-compatible implementation that
returns the single primary `DataSource` for all requests.
+- **`JdbcMetaStoreManagerFactory`**: Overwrites the resolution logic to use
the `DataSourceResolver`.
+- **`JdbcBasePersistenceImpl`**: Manages three separate `DatasourceOperations`
objects.
+
+## Schema Management and Evolution
+
+### Functional SQL Script Splitting
+To support hosting different workloads on different databases, the monolithic
`schema-vX.sql` scripts are split into functional components:
+- `schema-vX-metastore.sql`: Definitions for `polaris_entities`,
`polaris_grant_records`, and `polaris_schema_version`.
+- `schema-vX-metrics.sql`: Definitions for `polaris_metrics_scan` and
`polaris_metrics_commit`.
+- `schema-vX-events.sql`: Definitions for `polaris_events`.
+
+### Version Authority
+The **Metastore** remains the single source of truth for the realm's logical
schema version. The `polaris_schema_version` table is exclusively maintained
within the `METASTORE` data source. All associated stores (Metrics, Events) are
expected to be physically compatible with this detected version.
+
+## Operational Behaviors
+
+### Bootstrap
+When a realm is bootstrapped via
`JdbcMetaStoreManagerFactory.bootstrapRealms()`:
+1. **Resolution**: The `DataSourceResolver` is called for each `StoreType`
(METASTORE, METRICS, EVENTS).
+2. **Initialization**: Each resolved data source is initialized with its
specific functional SQL script.
+3. **Idempotency**: If all three `StoreType` mappings point to the same
physical database, the scripts are applied sequentially. The DDL is designed to
be idempotent to prevent errors during this "merged" initialization.
+
+### Purge (Cleanup)
+When a realm is purged:
+1. The `purge()` operation is initiated.
+2. The `JdbcBasePersistenceImpl.deleteAll()` method is triggered.
+3. **Multi-Store Cleanup**: `deleteAll()` executes `DELETE` commands
targeting the specific `realm_id` across all three `DatasourceOperations`:
+- `metastoreOps`: Clears entities, grants, secrets, and policy mappings.
+- `metricsOps`: Clears `scan_metrics_report` and `commit_metrics_report`.
+- `eventOps`: Clears the `events` table.
+4. This ensures that all data across all three potential data sources is
cleaned up for the specified `realm_id`.
+
+## Schema Upgrades
+Upgrading a Polaris deployment from version N to N+1 involves:
+1. **Detection**: The service detects that the `metastore`'s
`polaris_schema_version` is below the requested level.
+2. **Execution**: The migration logic resolves each `StoreType` and applies
the relevant "vN-to-vN+1" upgrade script.
Review Comment:
Thanks, @Subham-KRLX !
@flyrain , @singhpk234 : What is your take on the proposed design?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r3025469557
##
site/content/in-dev/unreleased/proposals/jdbc-multi-datasource.md:
##
@@ -0,0 +1,90 @@
+
+# Design: Multi-DataSource support in JDBC persistence
+
+## Goal
+The goal of this design is to decouple `DataSource` selection from the core
JDBC persistence logic. This allows for:
+1. **Workload Isolation**: Separating the Metastore, Metrics reporting, and
Event logging into different physical databases or connection pools.
+2. **Scalable Multi-Tenancy**: Enabling per-realm (tenant) routing to support
large-scale deployments.
+
+## Core Interface: `DataSourceResolver`
+A service interface designed to resolve the correct `DataSource` based on the
workload's metadata.
+
+```java
+public interface DataSourceResolver {
+ enum StoreType {
+METASTORE,
+METRICS,
+EVENTS
+ }
+
+ DataSource resolve(RealmContext realmContext, StoreType storeType);
+}
+```
+
+### Key Components
+- **`DataSourceResolver`**: SPI for resolution logic.
+- **`DefaultDataSourceResolver`**: Backward-compatible implementation that
returns the single primary `DataSource` for all requests.
+- **`JdbcMetaStoreManagerFactory`**: Overwrites the resolution logic to use
the `DataSourceResolver`.
+- **`JdbcBasePersistenceImpl`**: Manages three separate `DatasourceOperations`
objects.
+
+## Schema Management and Evolution
+
+### Functional SQL Script Splitting
+To support hosting different workloads on different databases, the monolithic
`schema-vX.sql` scripts are split into functional components:
+- `schema-vX-metastore.sql`: Definitions for `polaris_entities`,
`polaris_grant_records`, and `polaris_schema_version`.
+- `schema-vX-metrics.sql`: Definitions for `polaris_metrics_scan` and
`polaris_metrics_commit`.
+- `schema-vX-events.sql`: Definitions for `polaris_events`.
+
+### Version Authority
+The **Metastore** remains the single source of truth for the realm's logical
schema version. The `polaris_schema_version` table is exclusively maintained
within the `METASTORE` data source. All associated stores (Metrics, Events) are
expected to be physically compatible with this detected version.
+
+## Operational Behaviors
+
+### Bootstrap
+When a realm is bootstrapped via
`JdbcMetaStoreManagerFactory.bootstrapRealms()`:
+1. **Resolution**: The `DataSourceResolver` is called for each `StoreType`
(METASTORE, METRICS, EVENTS).
+2. **Initialization**: Each resolved data source is initialized with its
specific functional SQL script.
+3. **Idempotency**: If all three `StoreType` mappings point to the same
physical database, the scripts are applied sequentially. The DDL is designed to
be idempotent to prevent errors during this "merged" initialization.
+
+### Purge (Cleanup)
+When a realm is purged:
+1. The `purge()` operation is initiated.
+2. The `JdbcBasePersistenceImpl.deleteAll()` method is triggered.
+3. **Multi-Store Cleanup**: `deleteAll()` executes `DELETE` commands
targeting the specific `realm_id` across all three `DatasourceOperations`:
+- `metastoreOps`: Clears entities, grants, secrets, and policy mappings.
+- `metricsOps`: Clears `scan_metrics_report` and `commit_metrics_report`.
+- `eventOps`: Clears the `events` table.
+4. This ensures that all data across all three potential data sources is
cleaned up for the specified `realm_id`.
+
+## Schema Upgrades
+Upgrading a Polaris deployment from version N to N+1 involves:
+1. **Detection**: The service detects that the `metastore`'s
`polaris_schema_version` is below the requested level.
+2. **Execution**: The migration logic resolves each `StoreType` and applies
the relevant "vN-to-vN+1" upgrade script.
Review Comment:
I have refactored the Multi-DataSource JDBC implementation to fully address
the feedback from @dimas-b. This update introduces selective bootstrap control
via new CLI flags (--metastore, --metrics, --events), independent schema
versioning for each store type, and moves schema upgrades to the explicit
administrative bootstrap flow Additionally I have added null-safety to
JdbcBasePersistenceImpl to support heterogeneous storage for metrics and
events, and the design proposal has been finalized and moved to
site/content/in-dev/unreleased/proposals/jdbc-multi-datasource.md.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r3023786198
##
site/content/in-dev/unreleased/metastores/jdbc-multi-datasource.md:
##
@@ -0,0 +1,90 @@
+
+# Design: Multi-DataSource support in JDBC persistence
Review Comment:
Should this perhaps be under `unreleased/proposals`?.. e.g. like in this
proposal using GH PR and `.md` files: #3924
I personally do not mind having the proposal doc in the same PR as the
implementation, but we need to be careful with reviews so as to settle on the
general proposal first and proceed to code changes later (to avoid unnecessary
work).
##
site/content/in-dev/unreleased/metastores/jdbc-multi-datasource.md:
##
@@ -0,0 +1,90 @@
+
+# Design: Multi-DataSource support in JDBC persistence
+
+## Goal
+The goal of this design is to decouple `DataSource` selection from the core
JDBC persistence logic. This allows for:
+1. **Workload Isolation**: Separating the Metastore, Metrics reporting, and
Event logging into different physical databases or connection pools.
+2. **Scalable Multi-Tenancy**: Enabling per-realm (tenant) routing to support
large-scale deployments.
+
+## Core Interface: `DataSourceResolver`
+A service interface designed to resolve the correct `DataSource` based on the
workload's metadata.
+
+```java
+public interface DataSourceResolver {
+ enum StoreType {
+METASTORE,
+METRICS,
+EVENTS
+ }
+
+ DataSource resolve(RealmContext realmContext, StoreType storeType);
+}
+```
+
+### Key Components
+- **`DataSourceResolver`**: SPI for resolution logic.
+- **`DefaultDataSourceResolver`**: Backward-compatible implementation that
returns the single primary `DataSource` for all requests.
+- **`JdbcMetaStoreManagerFactory`**: Overwrites the resolution logic to use
the `DataSourceResolver`.
+- **`JdbcBasePersistenceImpl`**: Manages three separate `DatasourceOperations`
objects.
+
+## Schema Management and Evolution
+
+### Functional SQL Script Splitting
+To support hosting different workloads on different databases, the monolithic
`schema-vX.sql` scripts are split into functional components:
+- `schema-vX-metastore.sql`: Definitions for `polaris_entities`,
`polaris_grant_records`, and `polaris_schema_version`.
+- `schema-vX-metrics.sql`: Definitions for `polaris_metrics_scan` and
`polaris_metrics_commit`.
+- `schema-vX-events.sql`: Definitions for `polaris_events`.
+
+### Version Authority
+The **Metastore** remains the single source of truth for the realm's logical
schema version. The `polaris_schema_version` table is exclusively maintained
within the `METASTORE` data source. All associated stores (Metrics, Events) are
expected to be physically compatible with this detected version.
+
+## Operational Behaviors
+
+### Bootstrap
+When a realm is bootstrapped via
`JdbcMetaStoreManagerFactory.bootstrapRealms()`:
+1. **Resolution**: The `DataSourceResolver` is called for each `StoreType`
(METASTORE, METRICS, EVENTS).
Review Comment:
I'd think the bootstrap process should indicate which of the possible store
types to initialize. Ultimately this is a the Admin user's decision, I think
(defaulting to all).
The rationale is to not create tables that the Admin user knows are not
going to be used (reducing database maintenance burden by not creating "dead"
tables).
##
site/content/in-dev/unreleased/metastores/jdbc-multi-datasource.md:
##
@@ -0,0 +1,90 @@
+
+# Design: Multi-DataSource support in JDBC persistence
+
+## Goal
+The goal of this design is to decouple `DataSource` selection from the core
JDBC persistence logic. This allows for:
+1. **Workload Isolation**: Separating the Metastore, Metrics reporting, and
Event logging into different physical databases or connection pools.
+2. **Scalable Multi-Tenancy**: Enabling per-realm (tenant) routing to support
large-scale deployments.
+
+## Core Interface: `DataSourceResolver`
+A service interface designed to resolve the correct `DataSource` based on the
workload's metadata.
+
+```java
+public interface DataSourceResolver {
+ enum StoreType {
+METASTORE,
+METRICS,
+EVENTS
+ }
+
+ DataSource resolve(RealmContext realmContext, StoreType storeType);
+}
+```
+
+### Key Components
+- **`DataSourceResolver`**: SPI for resolution logic.
+- **`DefaultDataSourceResolver`**: Backward-compatible implementation that
returns the single primary `DataSource` for all requests.
+- **`JdbcMetaStoreManagerFactory`**: Overwrites the resolution logic to use
the `DataSourceResolver`.
+- **`JdbcBasePersistenceImpl`**: Manages three separate `DatasourceOperations`
objects.
+
+## Schema Management and Evolution
+
+### Functional SQL Script Splitting
+To support hosting different workloads on different databases, the monolithic
`schema-vX.sql` scripts are split into functional components:
+- `schema-vX-metastore.sql`: Definitions for `polaris_entities`,
`polaris_grant_record
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4168927554 @flyrain, @flyingImer, @dimas-b Just wanted to let you know that I have updated the PR with the fixes we discussed and have officially sent the design proposal for Multi-DataSource support to the [email protected] mailing list for broader community feedback You can find the design document in the repository here: [jdbc-multi-datasource.md](https://github.com/Subham-KRLX/polaris/blob/feat/multi-datasource-support/site/content/in-dev/unreleased/metastores/jdbc-multi-datasource.md) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r3020548194
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##
@@ -154,32 +167,39 @@ public synchronized Map
bootstrapRealms(
for (String realm : bootstrapOptions.realms()) {
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
-DatasourceOperations datasourceOperations = getDatasourceOperations();
-int currentSchemaVersion =
-JdbcBasePersistenceImpl.loadSchemaVersion(datasourceOperations,
true);
+DatasourceOperations metastoreOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METASTORE);
+DatasourceOperations metricsOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METRICS);
+DatasourceOperations eventOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.EVENTS);
+
+int currentSchemaVersion =
JdbcBasePersistenceImpl.loadSchemaVersion(metastoreOps, true);
int requestedSchemaVersion =
JdbcBootstrapUtils.getRequestedSchemaVersion(bootstrapOptions);
int effectiveSchemaVersion =
JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(
-datasourceOperations.getDatabaseType(),
+metastoreOps.getDatabaseType(),
currentSchemaVersion,
requestedSchemaVersion,
-
JdbcBasePersistenceImpl.entityTableExists(datasourceOperations));
+JdbcBasePersistenceImpl.entityTableExists(metastoreOps));
LOGGER.info(
"Effective schema version: {} for bootstrapping realm: {}",
effectiveSchemaVersion,
realm);
+
try {
- // Run the set-up script to create the tables.
- datasourceOperations.executeScript(
- datasourceOperations
- .getDatabaseType()
- .openInitScriptResource(effectiveSchemaVersion));
+ // Run the set-up script to create the tables on all data sources.
+ metastoreOps.executeScript(
+
metastoreOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
+ metricsOps.executeScript(
+
metricsOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
+ eventOps.executeScript(
+
eventOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
Review Comment:
I have addressed your feedback by drafting a formal design document in
docs/metastores/jdbc-multi-datasource.md that covers schema evolution,
upgrades, bootstrap, and purge behavior. Along with the documentation I have
refined the purge implementation to ensure complete cleanup across isolated
data sources and resolved the rat task configuration issue to fix the build
blockers. I will share the design doc on the community mailing list today for
further review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
flyrain commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r3017340288
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##
@@ -154,32 +167,39 @@ public synchronized Map
bootstrapRealms(
for (String realm : bootstrapOptions.realms()) {
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
-DatasourceOperations datasourceOperations = getDatasourceOperations();
-int currentSchemaVersion =
-JdbcBasePersistenceImpl.loadSchemaVersion(datasourceOperations,
true);
+DatasourceOperations metastoreOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METASTORE);
+DatasourceOperations metricsOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METRICS);
+DatasourceOperations eventOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.EVENTS);
+
+int currentSchemaVersion =
JdbcBasePersistenceImpl.loadSchemaVersion(metastoreOps, true);
int requestedSchemaVersion =
JdbcBootstrapUtils.getRequestedSchemaVersion(bootstrapOptions);
int effectiveSchemaVersion =
JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(
-datasourceOperations.getDatabaseType(),
+metastoreOps.getDatabaseType(),
currentSchemaVersion,
requestedSchemaVersion,
-
JdbcBasePersistenceImpl.entityTableExists(datasourceOperations));
+JdbcBasePersistenceImpl.entityTableExists(metastoreOps));
LOGGER.info(
"Effective schema version: {} for bootstrapping realm: {}",
effectiveSchemaVersion,
realm);
+
try {
- // Run the set-up script to create the tables.
- datasourceOperations.executeScript(
- datasourceOperations
- .getDatabaseType()
- .openInitScriptResource(effectiveSchemaVersion));
+ // Run the set-up script to create the tables on all data sources.
+ metastoreOps.executeScript(
+
metastoreOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
+ metricsOps.executeScript(
+
metricsOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
+ eventOps.executeScript(
+
eventOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
Review Comment:
Thanks @Subham-KRLX for continuing to work on this. As I mentioned in the
dev mailing thread, supporting multiple data sources has implications for
schema evolution, Polaris version upgrades, as well as bootstrap and purge
behavior. We should think this through carefully to ensure that existing
behavior remains intact and current users are not impacted. Would you mind
drafting a design doc that covers these aspects? That would allow the Polaris
community to review the proposal and help us move forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4160031681 I finalized the PR by splitting the v4 SQL schema for workload isolation and moving CDI producers to the JDBC module, as requested including the rat audit and spotless formatting, with all 163/163 tests passing in :polaris-relational-jdbc. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r3013445832
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##
@@ -154,32 +167,39 @@ public synchronized Map
bootstrapRealms(
for (String realm : bootstrapOptions.realms()) {
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
-DatasourceOperations datasourceOperations = getDatasourceOperations();
-int currentSchemaVersion =
-JdbcBasePersistenceImpl.loadSchemaVersion(datasourceOperations,
true);
+DatasourceOperations metastoreOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METASTORE);
+DatasourceOperations metricsOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METRICS);
+DatasourceOperations eventOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.EVENTS);
+
+int currentSchemaVersion =
JdbcBasePersistenceImpl.loadSchemaVersion(metastoreOps, true);
int requestedSchemaVersion =
JdbcBootstrapUtils.getRequestedSchemaVersion(bootstrapOptions);
int effectiveSchemaVersion =
JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(
-datasourceOperations.getDatabaseType(),
+metastoreOps.getDatabaseType(),
currentSchemaVersion,
requestedSchemaVersion,
-
JdbcBasePersistenceImpl.entityTableExists(datasourceOperations));
+JdbcBasePersistenceImpl.entityTableExists(metastoreOps));
LOGGER.info(
"Effective schema version: {} for bootstrapping realm: {}",
effectiveSchemaVersion,
realm);
+
try {
- // Run the set-up script to create the tables.
- datasourceOperations.executeScript(
- datasourceOperations
- .getDatabaseType()
- .openInitScriptResource(effectiveSchemaVersion));
+ // Run the set-up script to create the tables on all data sources.
+ metastoreOps.executeScript(
+
metastoreOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
+ metricsOps.executeScript(
+
metricsOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
+ eventOps.executeScript(
+
eventOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
Review Comment:
@flyingImer, @dimas-b, @singhpk234, and @flyrain, thank you for the feedback
on the DataSourceResolver SPI To strike a balance between architectural
consistency and immediate scope I have kept RealmContext in the resolve()
signature to align with existing Polaris interfaces like
RealmConfigurationSource and TokenBucketFactory This ensures the SPI is
future-proof and maintains a standard extensibility hook while the
DefaultDataSourceResolver remains simple by unconditionally returning the
default DataSource (using RealmContext only for logging) the focus of this PR
remains workload isolation via StoreType and I have now fixed the CI failures
by committing the necessary SQL script splits and documentation updates. I
believe this approach manages the long-term "routing story" concerns while
delivering the immediate value of separate metastore, metrics, and events
connection pools.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r3012538947
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##
@@ -154,32 +167,39 @@ public synchronized Map
bootstrapRealms(
for (String realm : bootstrapOptions.realms()) {
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
-DatasourceOperations datasourceOperations = getDatasourceOperations();
-int currentSchemaVersion =
-JdbcBasePersistenceImpl.loadSchemaVersion(datasourceOperations,
true);
+DatasourceOperations metastoreOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METASTORE);
+DatasourceOperations metricsOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METRICS);
+DatasourceOperations eventOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.EVENTS);
+
+int currentSchemaVersion =
JdbcBasePersistenceImpl.loadSchemaVersion(metastoreOps, true);
int requestedSchemaVersion =
JdbcBootstrapUtils.getRequestedSchemaVersion(bootstrapOptions);
int effectiveSchemaVersion =
JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(
-datasourceOperations.getDatabaseType(),
+metastoreOps.getDatabaseType(),
currentSchemaVersion,
requestedSchemaVersion,
-
JdbcBasePersistenceImpl.entityTableExists(datasourceOperations));
+JdbcBasePersistenceImpl.entityTableExists(metastoreOps));
LOGGER.info(
"Effective schema version: {} for bootstrapping realm: {}",
effectiveSchemaVersion,
realm);
+
try {
- // Run the set-up script to create the tables.
- datasourceOperations.executeScript(
- datasourceOperations
- .getDatabaseType()
- .openInitScriptResource(effectiveSchemaVersion));
+ // Run the set-up script to create the tables on all data sources.
+ metastoreOps.executeScript(
+
metastoreOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
+ metricsOps.executeScript(
+
metricsOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
+ eventOps.executeScript(
+
eventOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
Review Comment:
@flyrain @singhpk234 : WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2999243423
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##
@@ -154,32 +167,39 @@ public synchronized Map
bootstrapRealms(
for (String realm : bootstrapOptions.realms()) {
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
-DatasourceOperations datasourceOperations = getDatasourceOperations();
-int currentSchemaVersion =
-JdbcBasePersistenceImpl.loadSchemaVersion(datasourceOperations,
true);
+DatasourceOperations metastoreOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METASTORE);
+DatasourceOperations metricsOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METRICS);
+DatasourceOperations eventOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.EVENTS);
+
+int currentSchemaVersion =
JdbcBasePersistenceImpl.loadSchemaVersion(metastoreOps, true);
int requestedSchemaVersion =
JdbcBootstrapUtils.getRequestedSchemaVersion(bootstrapOptions);
int effectiveSchemaVersion =
JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(
-datasourceOperations.getDatabaseType(),
+metastoreOps.getDatabaseType(),
currentSchemaVersion,
requestedSchemaVersion,
-
JdbcBasePersistenceImpl.entityTableExists(datasourceOperations));
+JdbcBasePersistenceImpl.entityTableExists(metastoreOps));
LOGGER.info(
"Effective schema version: {} for bootstrapping realm: {}",
effectiveSchemaVersion,
realm);
+
try {
- // Run the set-up script to create the tables.
- datasourceOperations.executeScript(
- datasourceOperations
- .getDatabaseType()
- .openInitScriptResource(effectiveSchemaVersion));
+ // Run the set-up script to create the tables on all data sources.
+ metastoreOps.executeScript(
+
metastoreOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
+ metricsOps.executeScript(
+
metricsOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
+ eventOps.executeScript(
+
eventOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
Review Comment:
Agreed I have updated the PR to split the v4 SQL schema into specialized
scripts -metastore, -events, and -metrics. The bootstrap process now executes
only the relevant DDL sub-section for each isolated data source, ensuring they
each only contain the tables necessary for their specific workload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960: URL: https://github.com/apache/polaris/pull/3960#discussion_r2991339125 ## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java: ## @@ -103,15 +107,22 @@ private void initializeForRealm( // determine schemaVersion once per realm final int schemaVersion = JdbcBasePersistenceImpl.loadSchemaVersion( -datasourceOperations, +metastoreOps, realmConfig.getConfig(BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE)); +DatasourceOperations metricsOps = +getDatasourceOperations(realmContext, DataSourceResolver.StoreType.METRICS); Review Comment: thx - a posted a separate follow-up comment. I think we can resolve this thread. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2991336294
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##
@@ -154,32 +167,39 @@ public synchronized Map
bootstrapRealms(
for (String realm : bootstrapOptions.realms()) {
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
-DatasourceOperations datasourceOperations = getDatasourceOperations();
-int currentSchemaVersion =
-JdbcBasePersistenceImpl.loadSchemaVersion(datasourceOperations,
true);
+DatasourceOperations metastoreOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METASTORE);
+DatasourceOperations metricsOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METRICS);
+DatasourceOperations eventOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.EVENTS);
+
+int currentSchemaVersion =
JdbcBasePersistenceImpl.loadSchemaVersion(metastoreOps, true);
int requestedSchemaVersion =
JdbcBootstrapUtils.getRequestedSchemaVersion(bootstrapOptions);
int effectiveSchemaVersion =
JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(
-datasourceOperations.getDatabaseType(),
+metastoreOps.getDatabaseType(),
currentSchemaVersion,
requestedSchemaVersion,
-
JdbcBasePersistenceImpl.entityTableExists(datasourceOperations));
+JdbcBasePersistenceImpl.entityTableExists(metastoreOps));
LOGGER.info(
"Effective schema version: {} for bootstrapping realm: {}",
effectiveSchemaVersion,
realm);
+
try {
- // Run the set-up script to create the tables.
- datasourceOperations.executeScript(
- datasourceOperations
- .getDatabaseType()
- .openInitScriptResource(effectiveSchemaVersion));
+ // Run the set-up script to create the tables on all data sources.
+ metastoreOps.executeScript(
+
metastoreOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
+ metricsOps.executeScript(
+
metricsOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
+ eventOps.executeScript(
+
eventOps.getDatabaseType().openInitScriptResource(effectiveSchemaVersion));
Review Comment:
I believe in each of those bootstrap calls should execute a sub-section of
the DDL relevant to the sub-system (metastore, metrics, events).
@flyrain @singhpk234 : WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2986287176
##
persistence/nosql/persistence/api/src/main/java/org/apache/polaris/persistence/nosql/api/exceptions/CommitTraversalLimitExceededException.java:
##
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.nosql.api.exceptions;
+
+/**
+ * Thrown when the commit history traversal exceeds the configured limit. This
is a safeguard
+ * against unbounded resource consumption.
+ */
+public class CommitTraversalLimitExceededException extends
PersistenceException {
Review Comment:
You are right this was an unrelated addition I have reverted the changes and
removed the exception class from this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960: URL: https://github.com/apache/polaris/pull/3960#discussion_r2986283686 ## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java: ## @@ -103,15 +107,22 @@ private void initializeForRealm( // determine schemaVersion once per realm final int schemaVersion = JdbcBasePersistenceImpl.loadSchemaVersion( -datasourceOperations, +metastoreOps, realmConfig.getConfig(BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE)); +DatasourceOperations metricsOps = +getDatasourceOperations(realmContext, DataSourceResolver.StoreType.METRICS); Review Comment: Agreed I removed dataSourceResolverType() from the core RelationalJdbcConfiguration and moved JdbcCdiProducers to the runtime-common module to keep the persistence layer decoupled from configuration details. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960: URL: https://github.com/apache/polaris/pull/3960#discussion_r2986276520 ## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java: ## @@ -103,15 +107,22 @@ private void initializeForRealm( // determine schemaVersion once per realm final int schemaVersion = JdbcBasePersistenceImpl.loadSchemaVersion( -datasourceOperations, +metastoreOps, realmConfig.getConfig(BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE)); +DatasourceOperations metricsOps = +getDatasourceOperations(realmContext, DataSourceResolver.StoreType.METRICS); Review Comment: I have updated bootstrapRealms to resolve and initialize all three store types (METASTORE, METRICS, and EVENTS) so that the setup script is correctly applied across all data sources during bootstrap. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2982275497
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##
@@ -121,10 +125,12 @@ private void initializeForRealm(
metaStoreManagerMap.put(realmId, metaStoreManager);
}
- public DatasourceOperations getDatasourceOperations() {
+ public DatasourceOperations getDatasourceOperations(RealmContext
realmContext) {
DatasourceOperations databaseOperations;
try {
- databaseOperations = new DatasourceOperations(dataSource.get(),
relationalJdbcConfiguration);
+ DataSource resolvedDs =
+ dataSourceResolver.resolve(realmContext,
DataSourceResolver.StoreType.METASTORE);
Review Comment:
Thanks for the update :+1: I've posted more specific comments in new
threads... closing this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2982252466
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##
@@ -103,15 +107,22 @@ private void initializeForRealm(
// determine schemaVersion once per realm
final int schemaVersion =
JdbcBasePersistenceImpl.loadSchemaVersion(
-datasourceOperations,
+metastoreOps,
realmConfig.getConfig(BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE));
+DatasourceOperations metricsOps =
+getDatasourceOperations(realmContext,
DataSourceResolver.StoreType.METRICS);
Review Comment:
We obtain a dedicated `DataSource` for metrics, but the bootstrap method
(line 171 and below) runs the schema DLL always on the `METASTORE` `DataSource`.
This means that while the code makes the appearance that
`DataSourceResolver` may return a different data source for metrics, in
practice it will likely end up getting runtime errors due to missing tables.
WDYT?
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcConfiguration.java:
##
@@ -35,4 +35,7 @@ public interface RelationalJdbcConfiguration {
* the JDBC connection metadata. Supported values: "postgresql",
"cockroachdb", "h2"
*/
Optional databaseType();
+
+ /** The type of the {@link DataSourceResolver} to use. */
+ String dataSourceResolverType();
Review Comment:
`JdbcCdiProducers` should be able to use a different config class (or simply
a local config parameter).
I believe this config method belongs to the CDI code, it is not be relevant
to JDBC Persistence implementations directly since they receive the
`DataSource` through injection and do not have to know how it was resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2982155121
##
persistence/nosql/persistence/api/src/main/java/org/apache/polaris/persistence/nosql/api/exceptions/CommitTraversalLimitExceededException.java:
##
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.nosql.api.exceptions;
+
+/**
+ * Thrown when the commit history traversal exceeds the configured limit. This
is a safeguard
+ * against unbounded resource consumption.
+ */
+public class CommitTraversalLimitExceededException extends
PersistenceException {
Review Comment:
Is it related to the purpose on this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2979089082
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##
@@ -121,10 +125,12 @@ private void initializeForRealm(
metaStoreManagerMap.put(realmId, metaStoreManager);
}
- public DatasourceOperations getDatasourceOperations() {
+ public DatasourceOperations getDatasourceOperations(RealmContext
realmContext) {
DatasourceOperations databaseOperations;
try {
- databaseOperations = new DatasourceOperations(dataSource.get(),
relationalJdbcConfiguration);
+ DataSource resolvedDs =
+ dataSourceResolver.resolve(realmContext,
DataSourceResolver.StoreType.METASTORE);
Review Comment:
@dimas-b You are right that the previous version reused the same DataSource
for all operations I have updated the PR to fully implement the foundation for
workload isolation: JdbcMetaStoreManagerFactory now resolves three distinct
DataSource instances (for METASTORE, METRICS, and EVENTS) and provides them as
isolated metastoreOps, metricsOps, and eventOps to the persistence
implementation. This makes the StoreType parameter meaningful and establishes
the connection pool separation discussed as a high priority earlier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2979089082
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##
@@ -121,10 +125,12 @@ private void initializeForRealm(
metaStoreManagerMap.put(realmId, metaStoreManager);
}
- public DatasourceOperations getDatasourceOperations() {
+ public DatasourceOperations getDatasourceOperations(RealmContext
realmContext) {
DatasourceOperations databaseOperations;
try {
- databaseOperations = new DatasourceOperations(dataSource.get(),
relationalJdbcConfiguration);
+ DataSource resolvedDs =
+ dataSourceResolver.resolve(realmContext,
DataSourceResolver.StoreType.METASTORE);
Review Comment:
@dimas-b. You are right that the previous version reused the same DataSource
for all operations. I've updated the PR to fully implement the foundation for
workload isolation: JdbcMetaStoreManagerFactory now resolves three distinct
DataSource instances (for METASTORE, METRICS, and EVENTS) and provides them as
isolated metastoreOps, metricsOps, and eventOps to the persistence
implementation. This makes the StoreType parameter meaningful and establishes
the connection pool separation discussed as a high priority earlier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2977097230
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store
+ * type. This enables
+ * isolating different workloads (e.g., entity metadata vs metrics vs events)
+ * into different
+ * physical databases or connection pools.
+ */
+public interface DataSourceResolver {
+
+ /** The type of store representing the workload pattern. */
+ enum StoreType {
+METASTORE,
+METRICS,
+EVENTS
+ }
+
+ /**
+ * Resolves the DataSource for a given realm and store type.
+ *
+ * @param realmContext the realm context
+ * @param storeTypethe type of store (e.g., main, metrics, events)
+ * @return the resolved DataSource
+ */
+ DataSource resolve(RealmContext realmContext, StoreType storeType);
Review Comment:
From my POB if we are to introduce `StoreType` into this
`DataSourceResolver`, we first have to isolate MetaStore, Metrics and Events
persistence use cases in current code, so that each of them will not reuse data
sources meant for the others.
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store
+ * type. This enables
+ * isolating different workloads (e.g., entity metadata vs metrics vs events)
+ * into different
+ * physical databases or connection pools.
+ */
+public interface DataSourceResolver {
+
+ /** The type of store representing the workload pattern. */
+ enum StoreType {
+METASTORE,
+METRICS,
+EVENTS
+ }
+
+ /**
+ * Resolves the DataSource for a given realm and store type.
+ *
+ * @param realmContext the realm context
+ * @param storeTypethe type of store (e.g., main, metrics, events)
+ * @return the resolved DataSource
+ */
+ DataSource resolve(RealmContext realmContext, StoreType storeType);
Review Comment:
From my POV if we are to introduce `StoreType` into this
`DataSourceResolver`, we first have to isolate MetaStore, Metrics and Events
persistence use cases in current code, so that each of them will not reuse data
sources meant for the others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2977084551
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##
@@ -121,10 +125,12 @@ private void initializeForRealm(
metaStoreManagerMap.put(realmId, metaStoreManager);
}
- public DatasourceOperations getDatasourceOperations() {
+ public DatasourceOperations getDatasourceOperations(RealmContext
realmContext) {
DatasourceOperations databaseOperations;
try {
- databaseOperations = new DatasourceOperations(dataSource.get(),
relationalJdbcConfiguration);
+ DataSource resolvedDs =
+ dataSourceResolver.resolve(realmContext,
DataSourceResolver.StoreType.METASTORE);
Review Comment:
This `DataSource` will be reused for metrics... TBH, I do not see the
rationale behind the `DataSourceResolver.StoreType.METASTORE` parameter here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4112815060 @Subham-KRLX : > I've updated the PR to re-instate StoreType (METASTORE, METRICS, EVENTS) [...] I do not see `StoreType` in the current diff of this PR... Did you forget to push? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4112803039 from CI: `ERROR: Configuration reference is out of date. Please run './gradlew :polaris-config-docs-site:copyConfigSectionsToSite' and commit the changes.` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4098362590 Thanks @adnanhemani, @flyrain, and @singhpk234. I completely agree that workload-based isolation (Metrics vs Events vs Metastore) is the highest priority. I have updated the PR to re-instate StoreType (METASTORE, METRICS, EVENTS) so the immediate focus is separating connection pools. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
adnanhemani commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4088266008 Agreed here with some of the comments asking what value is bringing realm-based datasource resolution into the codebase - especially with a shared JVM that is controlling the request flow. I am afraid that adding this type of functionality without any metrics or considering alternatives may bring unnecessary complication to the codebase. If a design doc is being written to support this use case, it would be good to include any such data there may exist that prove the need for this feature. I do think that we are all on the same page that workload-based isolation (different datasource for events, metrics, metadata, etc.) - it does make sense and we do need this feature sooner than later. I would also suggest starting the implementation from that angle instead. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4087576422 @flyrain, @singhpk234, and @dimas-b. I agree that a formal design document is appropriate and will prepare one that covers both the multi-tenancy requirements and the immediate need for workload isolation. To ensure a future-proof foundation I will re-add StoreType to the DataSourceResolver interface now so it supports separate routing for metastore, metrics, and events traffic right from the start. I will post the design doc for review shortly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
flyrain commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4086913176 As I said in the dev mailing list[1], introducing multiple data sources requires a proper design before jumping to a implementation. There are multiple factors to consider. We may gather more feedback given JDBC is widely used by the community. Blindly introduce a data source resolver doesn't make sense to me. I'd suggest to work on a design doc first. 1. https://lists.apache.org/thread/dvr22fqw45zd49sdw4027xrz4gq2xpbc -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4086748255 Re: DataSource / realm mapping at deployment time - I think this is a concern of the Polaris "owner". I believe Polaris as an OSS project should empower users to make customizations that fit their specific deployment choices. I believe Polaris should remain neutral / flexible and not prescribe any specific deployment strategies. @Subham-KRLX 's comments related to this resonate with my personal view on this matter. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4086728501 @Subham-KRLX @singhpk234 @flyingImer : > Regarding StoreType for workload isolation [...] If you prefer to resolve the workload isolation problem in code first, I'd support that. We could put this PR on hold, isolate MetaStore, Metrics, Idempotency, then resume work on the DataSource resolver. Going the other way is fine too :slightly_smiling_face: The end result is going to be the same (as least that how it see it). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2942193380
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcConfiguration.java:
##
@@ -35,4 +35,7 @@ public interface RelationalJdbcConfiguration {
* the JDBC connection metadata. Supported values: "postgresql",
"cockroachdb", "h2"
*/
Optional databaseType();
+
+ /** The type of the {@link DataSourceResolver} to use. */
+ String dataSourceResolverType();
Review Comment:
Does this method have to be here? The JDBC persistence code (configured via
this interface) does not actually call it (and does not need to call).
##
runtime/service/build.gradle.kts:
##
@@ -31,7 +31,7 @@ dependencies {
implementation(project(":polaris-api-iceberg-service"))
implementation(project(":polaris-api-catalog-service"))
- runtimeOnly(project(":polaris-relational-jdbc"))
+ implementation(project(":polaris-relational-jdbc"))
Review Comment:
I'd prefer to avoid making this dependency more prominent.
Ideally, only `runtime/server` should depend on `polaris-relational-jdbc`,
but keep the old runtime-only dep for now is fine too.
##
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java:
##
@@ -228,6 +230,14 @@ public MetaStoreManagerFactory metaStoreManagerFactory(
return
metaStoreManagerFactories.select(Identifier.Literal.of(config.type())).get();
}
+ @Produces
+ public DataSourceResolver dataSourceResolver(
Review Comment:
Should it be `@ApplicationScoped`? I do not believe it can change in runtime.
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import io.quarkus.arc.DefaultBean;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
+ * single default {@link DataSource}. This implementation acts as a fallback;
downstream users can
+ * provide their own {@link DataSourceResolver} bean to implement custom
routing logic.
+ */
+@ApplicationScoped
+@DefaultBean
+public class DefaultDataSourceResolver implements DataSourceResolver {
Review Comment:
Looking at the evolution of this PR (and this class specifically), I think
it's probably fine to move CDI/quarkus-related code into the
`polaris-relational-jdbc` module.
##
runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/QuarkusRelationalJdbcConfiguration.java:
##
@@ -19,7 +19,15 @@
package org.apache.polaris.quarkus.common.config.jdbc;
import io.smallrye.config.ConfigMapping;
+import io.smallrye.config.WithDefault;
+import io.smallrye.config.WithName;
import
org.apache.polaris.persistence.relational.jdbc.RelationalJdbcConfiguration;
@ConfigMapping(prefix = "polaris.persistence.relational.jdbc")
-public interface QuarkusRelationalJdbcConfiguration extends
RelationalJdbcConfiguration {}
+public interface QuarkusRelationalJdbcConfiguration extends
RelationalJdbcConfiguration {
Review Comment:
As noted in other comment threads, this class should probably go to
`polaris-relational-jdbc`, where the it would be co-located with CDI producers
for JDBC stuff.
##
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java:
##
@@ -228,6 +230,14 @@ public MetaStoreManagerFactory metaStoreManagerFactory(
return
metaStoreManagerFactories.select(Identifier.Literal.of(config.type())).get();
}
+ @Produces
+ public DataSourceResolver dataSourceResolver(
+ RelationalJdbcConfiguration jdbcConfig,
+ @Any I
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2940756309
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcConfiguration.java:
##
@@ -35,4 +35,9 @@ public interface RelationalJdbcConfiguration {
* the JDBC connection metadata. Supported values: "postgresql",
"cockroachdb", "h2"
*/
Optional databaseType();
+
+ /** The type of the {@link DataSourceResolver} to use. Defaults to
"default". */
+ default Optional dataSourceResolverType() {
+return Optional.of("default");
+ }
Review Comment:
I have applied the final dataSourceResolverType /
QuarkusRelationalJdbcConfiguration / ServiceProducers changes per review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
adutra commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2939510858
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcConfiguration.java:
##
@@ -35,4 +35,9 @@ public interface RelationalJdbcConfiguration {
* the JDBC connection metadata. Supported values: "postgresql",
"cockroachdb", "h2"
*/
Optional databaseType();
+
+ /** The type of the {@link DataSourceResolver} to use. Defaults to
"default". */
+ default Optional dataSourceResolverType() {
+return Optional.of("default");
+ }
Review Comment:
nit: if there is a known default, using `Optional` is generally superfluous.
Also, it would be better to leave this unimplemented here :
```java
String dataSourceResolverType();
```
But override it in `QuarkusRelationalJdbcConfiguration` as follows:
```java
@ConfigMapping(prefix = "polaris.persistence.relational.jdbc")
public interface QuarkusRelationalJdbcConfiguration extends
RelationalJdbcConfiguration {
@WithDefault("default")
@Override
String dataSourceResolverType();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931997742
##
runtime/service/src/main/java/org/apache/polaris/service/persistence/PersistenceConfiguration.java:
##
@@ -37,4 +37,11 @@ public interface PersistenceConfiguration {
default boolean isAutoBootstrap() {
return autoBootstrapTypes().contains(type());
}
+
+ /**
+ * The type of the {@link
org.apache.polaris.persistence.relational.jdbc.DataSourceResolver} to
+ * use. Only applicable when using JDBC persistence.
+ */
+ @WithDefault("polaris")
+ String dataSourceResolver();
Review Comment:
@adutra @dimas-b Pushed the final updates:
1. Renamed to `@Identifier("default")`.
2. Isolated
[dataSourceResolverType](cci:1://file:///Users/subhamsangwan/polaris/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcConfiguration.java:38:2-41:3)
config into
[RelationalJdbcConfiguration](cci:2://file:///Users/subhamsangwan/polaris/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcConfiguration.java:22:0-42:1)
so non-JDBC isn't burdened.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931893666
##
runtime/service/src/main/java/org/apache/polaris/service/persistence/PersistenceConfiguration.java:
##
@@ -37,4 +37,11 @@ public interface PersistenceConfiguration {
default boolean isAutoBootstrap() {
return autoBootstrapTypes().contains(type());
}
+
+ /**
+ * The type of the {@link
org.apache.polaris.persistence.relational.jdbc.DataSourceResolver} to
+ * use. Only applicable when using JDBC persistence.
+ */
+ @WithDefault("polaris")
+ String dataSourceResolver();
Review Comment:
+1 to `@WithName("datasource-resolver.type")`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931881538
##
runtime/service/src/main/java/org/apache/polaris/service/persistence/PersistenceConfiguration.java:
##
@@ -37,4 +37,11 @@ public interface PersistenceConfiguration {
default boolean isAutoBootstrap() {
return autoBootstrapTypes().contains(type());
}
+
+ /**
+ * The type of the {@link
org.apache.polaris.persistence.relational.jdbc.DataSourceResolver} to
+ * use. Only applicable when using JDBC persistence.
+ */
+ @WithDefault("polaris")
+ String dataSourceResolver();
Review Comment:
@adutra Got it will use @Identifier("default") and
@WithName("datasource-resolver.type") with @WithDefault("default"). Pushing the
fix along with CI fixes shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931878212
##
runtime/service/src/main/java/org/apache/polaris/service/persistence/PersistenceConfiguration.java:
##
@@ -37,4 +37,11 @@ public interface PersistenceConfiguration {
default boolean isAutoBootstrap() {
return autoBootstrapTypes().contains(type());
}
+
+ /**
+ * The type of the {@link
org.apache.polaris.persistence.relational.jdbc.DataSourceResolver} to
+ * use. Only applicable when using JDBC persistence.
+ */
+ @WithDefault("polaris")
+ String dataSourceResolver();
Review Comment:
Would it be possible to isolate JDBC-specific config from the general
Persistence config?
I believe NoSQL persistence has some local config classes already.
`dataSourceResolver()` is only relevant to JDBC, so any Polaris deployments
that do not use JDBC, should not be burdened with this config, IMHO.
Upd: I believe it is best to resolve this in current PR. I know it's extra
work, but why introduce a config skew only to have to fix it in a follow-up PR
right after?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931878212
##
runtime/service/src/main/java/org/apache/polaris/service/persistence/PersistenceConfiguration.java:
##
@@ -37,4 +37,11 @@ public interface PersistenceConfiguration {
default boolean isAutoBootstrap() {
return autoBootstrapTypes().contains(type());
}
+
+ /**
+ * The type of the {@link
org.apache.polaris.persistence.relational.jdbc.DataSourceResolver} to
+ * use. Only applicable when using JDBC persistence.
+ */
+ @WithDefault("polaris")
+ String dataSourceResolver();
Review Comment:
Would it be possible to isolate JDBC-specific config from the general
Persistence config?
I believe NoSQL persistence has some local config classes already.
`dataSourceResolver()` is only relevant to JDBC, so any Polaris deployments
that do not use JDBC, should not be burdened with this config, IMHO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
adutra commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931861173
##
runtime/service/src/main/java/org/apache/polaris/service/persistence/PersistenceConfiguration.java:
##
@@ -37,4 +37,11 @@ public interface PersistenceConfiguration {
default boolean isAutoBootstrap() {
return autoBootstrapTypes().contains(type());
}
+
+ /**
+ * The type of the {@link
org.apache.polaris.persistence.relational.jdbc.DataSourceResolver} to
+ * use. Only applicable when using JDBC persistence.
+ */
+ @WithDefault("polaris")
+ String dataSourceResolver();
Review Comment:
BTW I think my example above is wrong, should be:
```java
@WithDefault("default")
@WithName("datasource-resolver.type")
String dataSourceResolver();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
adutra commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931855734
##
runtime/service/src/main/java/org/apache/polaris/service/persistence/PersistenceConfiguration.java:
##
@@ -37,4 +37,11 @@ public interface PersistenceConfiguration {
default boolean isAutoBootstrap() {
return autoBootstrapTypes().contains(type());
}
+
+ /**
+ * The type of the {@link
org.apache.polaris.persistence.relational.jdbc.DataSourceResolver} to
+ * use. Only applicable when using JDBC persistence.
+ */
+ @WithDefault("polaris")
+ String dataSourceResolver();
Review Comment:
Imho a follow-up is fine, because `DefaultDataSourceResolver` doesn't need
Agroal, so no need for a Quarkus module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931851029
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import io.quarkus.arc.DefaultBean;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
+ * single default {@link DataSource}. This implementation acts as a fallback;
downstream users can
+ * provide their own {@link DataSourceResolver} bean to implement custom
routing logic.
+ */
+@ApplicationScoped
+@DefaultBean
+public class DefaultDataSourceResolver implements DataSourceResolver {
Review Comment:
Only "general principles" kind of reason :slightly_smiling_face:
I tend to think it valuable to isolate runtime / CDI concerns from the basic
code / logic of a particular component like JDBC persistence. In that regard,
ideally, `polaris-relational-jdbc` could be reused in any runtime env. (Quarkus
or something else). `polaris-relational-jdbc-runtime` would have the code
specific to the way Polaris uses Quarkus.
I believe the NoSQL Persistence impl and the OPA Authorizer follow similar
principles (perhaps varying in degree, but similar in essence). I also proposed
a similar refactoring for the Admin tool in #3947.
I do not really know whether this matters for downstream Polaris users right
now :thinking: :shrug: In my personal experience, I find that it is much easier
to include another module into a build than struggle with excluding intruding
dependencies :sweat_smile:
I know some concerns were raised about module proliferation in Polaris, but
from my POV, Gradle is able to deal with a large number of modules very well,
so I personally do not see it as an issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931830018
##
runtime/service/src/main/java/org/apache/polaris/service/persistence/PersistenceConfiguration.java:
##
@@ -37,4 +37,11 @@ public interface PersistenceConfiguration {
default boolean isAutoBootstrap() {
return autoBootstrapTypes().contains(type());
}
+
+ /**
+ * The type of the {@link
org.apache.polaris.persistence.relational.jdbc.DataSourceResolver} to
+ * use. Only applicable when using JDBC persistence.
+ */
+ @WithDefault("polaris")
+ String dataSourceResolver();
Review Comment:
@adutra @dimas-b I will rename the identifier to "default" and update the
config property to polaris.persistence.datasource-resolver.type as suggested.
Regarding the broader discussion on module placement — I agree a dedicated
polaris-relational-jdbc-runtime module sounds like a clean approach for keeping
Quarkus/Agroal dependencies separate. Happy to tackle that as a follow-up or
incorporate it here if you'd prefer. Will push the fixes along with CI fixes
shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
adutra commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931803925
##
runtime/service/src/main/java/org/apache/polaris/service/persistence/PersistenceConfiguration.java:
##
@@ -37,4 +37,11 @@ public interface PersistenceConfiguration {
default boolean isAutoBootstrap() {
return autoBootstrapTypes().contains(type());
}
+
+ /**
+ * The type of the {@link
org.apache.polaris.persistence.relational.jdbc.DataSourceResolver} to
+ * use. Only applicable when using JDBC persistence.
+ */
+ @WithDefault("polaris")
+ String dataSourceResolver();
Review Comment:
Oh sorry - I read too quickly 😅
But to align with other similar cases, it would be good to turn this into
`polaris.persistence.datasource-resolver.type`:
```java
@WithDefault("default")
@WithName("polaris.persistence.datasource-resolver.type")
String dataSourceResolver();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
adutra commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931785165
##
runtime/service/src/main/java/org/apache/polaris/service/persistence/PersistenceConfiguration.java:
##
@@ -37,4 +37,11 @@ public interface PersistenceConfiguration {
default boolean isAutoBootstrap() {
return autoBootstrapTypes().contains(type());
}
+
+ /**
+ * The type of the {@link
org.apache.polaris.persistence.relational.jdbc.DataSourceResolver} to
+ * use. Only applicable when using JDBC persistence.
+ */
+ @WithDefault("polaris")
+ String dataSourceResolver();
Review Comment:
You don't need this new method, just use the existing `type()` method above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
adutra commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931780191
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import io.quarkus.arc.DefaultBean;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
+ * single default {@link DataSource}. This implementation acts as a fallback;
downstream users can
+ * provide their own {@link DataSourceResolver} bean to implement custom
routing logic.
+ */
+@ApplicationScoped
+@DefaultBean
+public class DefaultDataSourceResolver implements DataSourceResolver {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(DefaultDataSourceResolver.class);
+
+ private final Instance defaultDataSource;
+
+ @Inject
Review Comment:
OK makes sense 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
adutra commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931771436
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DefaultDataSourceResolver.java:
##
@@ -16,42 +16,38 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.quarkus.common.config.jdbc;
+package org.apache.polaris.persistence.relational.jdbc;
-import io.quarkus.arc.DefaultBean;
+import io.smallrye.common.annotation.Identifier;
import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Any;
import jakarta.enterprise.inject.Instance;
import jakarta.inject.Inject;
import javax.sql.DataSource;
import org.apache.polaris.core.context.RealmContext;
-import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
- * single default {@link DataSource}. This implementation acts as a fallback;
downstream users can
- * provide their own {@link DataSourceResolver} bean to implement custom
routing logic.
+ * Default implementation of {@link DataSourceResolver} that routes all realms
to a single default
+ * {@link DataSource}.
*/
@ApplicationScoped
-@DefaultBean
+@Identifier("polaris")
Review Comment:
Let's use `@Identifier("default")` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
adutra commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931763098
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import io.quarkus.arc.DefaultBean;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
+ * single default {@link DataSource}. This implementation acts as a fallback;
downstream users can
+ * provide their own {@link DataSourceResolver} bean to implement custom
routing logic.
+ */
+@ApplicationScoped
+@DefaultBean
+public class DefaultDataSourceResolver implements DataSourceResolver {
Review Comment:
Do we have a compelling reason not to turn `polaris-relational-jdbc` into a
Quarkus module? But if we do, I'm OK with `polaris-relational-jdbc-runtime`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931529076
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import io.quarkus.arc.DefaultBean;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
+ * single default {@link DataSource}. This implementation acts as a fallback;
downstream users can
+ * provide their own {@link DataSourceResolver} bean to implement custom
routing logic.
+ */
+@ApplicationScoped
+@DefaultBean
+public class DefaultDataSourceResolver implements DataSourceResolver {
Review Comment:
If we use `@Identifier` for selecting a specific `DataSourceResolver` impl.,
I suppose the producer code will go into `polaris-relational-jdbc-runtime` as
well.
Downstream alternatives will have the the option of reusing
`polaris-relational-jdbc-runtime` and configuring a custom identifier value, or
not including `polaris-relational-jdbc-runtime` and making a custom producer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931485520
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
+ * single default {@link DataSource}. This serves as both the production
default and the base for
+ * multi-datasource extensions.
+ */
+@ApplicationScoped
Review Comment:
Ok, let' use `@Identifier`. In this case, I suppose `@ApplicationScoped`
will move to the producer method, which will resolve the identifier based on
runtime config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931472760
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import io.quarkus.arc.DefaultBean;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
+ * single default {@link DataSource}. This implementation acts as a fallback;
downstream users can
+ * provide their own {@link DataSourceResolver} bean to implement custom
routing logic.
+ */
+@ApplicationScoped
+@DefaultBean
+public class DefaultDataSourceResolver implements DataSourceResolver {
Review Comment:
How about we add a new module like `polaris-relational-jdbc-runtime` with
Quarkus + Agroal dependencies?
The idea is for `polaris-relational-jdbc` to remain free from
Quarkus-specific dependencies (Jakarta annotations are ok). IIRC, we tried
doing something like that with JDBC persistence initially, but it probably did
not materialize completely :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931427340
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import io.quarkus.arc.DefaultBean;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
+ * single default {@link DataSource}. This implementation acts as a fallback;
downstream users can
+ * provide their own {@link DataSourceResolver} bean to implement custom
routing logic.
+ */
+@ApplicationScoped
+@DefaultBean
+public class DefaultDataSourceResolver implements DataSourceResolver {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(DefaultDataSourceResolver.class);
+
+ private final Instance defaultDataSource;
+
+ @Inject
Review Comment:
IIRC, there were some issues with bean not being available in non-JDBC
situations (e.g. tests) that's how I interpret @Subham-KRLX 's message :
https://github.com/apache/polaris/pull/3960#issuecomment-4045931648
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2931416509
##
runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import io.quarkus.arc.DefaultBean;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
+ * single default {@link DataSource}. This implementation acts as a fallback;
downstream users can
+ * provide their own {@link DataSourceResolver} bean to implement custom
routing logic.
+ */
+@ApplicationScoped
+@DefaultBean
Review Comment:
`@Identifier` works too. Yet, in this PR we do not have any alternatives, so
using no qualifiers should work too, AFAIK.
Custom implementations can use `@Alternative` + `@Priority` to override.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2930723983
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store type.
+ * Note: Currently this is implemented as a foundation for metastore routing,
not a
+ * full system-wide routing layer.
+ */
+public interface DataSourceResolver {
+
+ /** The type of store representing the workload pattern. */
+ enum StoreType {
+METASTORE
Review Comment:
Agreed, StoreType has been removed entirely in the latest push. As you
noted, it doesn't add value while JdbcBasePersistenceImpl handles both
MetaStore and MetricsPersistence. We can reintroduce it once the workloads are
actually isolated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on PR #3960:
URL: https://github.com/apache/polaris/pull/3960#issuecomment-4054548664
@adutra @dimas-b @flyingImer Thanks for the detailed feedback! I've pushed
an update that addresses all comments. StoreType has been removed entirely
since JdbcBasePersistenceImpl currently handles both MetaStore and
MetricsPersistence — we can reintroduce it once workloads are actually
isolated. @DefaultBean has been replaced with @Identifier("polaris") and a
producer method in ServiceProducers, following the standard Polaris pattern for
pluggable components. DefaultDataSourceResolver now lives in
polaris-relational-jdbc alongside its interface, and
JdbcMetaStoreManagerFactory injects DataSourceResolver directly. I kept
Instance (with @Any) in the default resolver to avoid
InactiveBeanException in non-JDBC profiles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
adutra commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2929822677
##
runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import io.quarkus.arc.DefaultBean;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
+ * single default {@link DataSource}. This implementation acts as a fallback;
downstream users can
+ * provide their own {@link DataSourceResolver} bean to implement custom
routing logic.
+ */
+@ApplicationScoped
+@DefaultBean
+public class DefaultDataSourceResolver implements DataSourceResolver {
Review Comment:
Actually the question of where to place implementations of this interface is
a tricky one.
When designing a true multi-datasource implementation, it will need the
Quarkus Agroal extension, and therefore, would likely have to live in
`runtime/service`.
It would look like this:
```java
@ApplicationScoped
@Identifier("per-realm")
public class PerRealmDataSourceResolver implements DataSourceResolver {
private static final Logger LOGGER =
LoggerFactory.getLogger(DefaultDataSourceResolver.class);
@Override
public DataSource resolve(RealmContext realmContext, StoreType storeType) {
String dataSourceName = findDataSourceName(realmContext, storeType);
LOGGER.debug(
"Using DataSource '{}' for realm '{}' and store '{}'",
dataSourceName,
realmContext.getRealmIdentifier(),
storeType);
return AgroalDataSourceUtil.dataSourceIfActive(dataSourceName)
.orElseThrow(
() ->
new IllegalStateException(
"DataSource '" + dataSourceName + "' is not active or
does not exist"));
}
private String findDataSourceName(RealmContext realmContext, StoreType
storeType) {
...
}
}
```
But the problem is that `polaris-relational-jdbc` is not in the compile
classpath of `runtime/service`, only in its runtime classpath... Another option
is to add the `quarkus-agroal` extension to `polaris-relational-jdbc` and make
it a quarkus module.
I think this needs some more thinking.
##
runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import io.quarkus.arc.DefaultBean;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementa
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2926185277
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store type.
+ * Note: Currently this is implemented as a foundation for metastore routing,
not a
+ * full system-wide routing layer.
+ */
+public interface DataSourceResolver {
+
+ /** The type of store representing the workload pattern. */
+ enum StoreType {
+METASTORE
Review Comment:
Sorry, I might have missed this before, but this enum appears to be rather
confusing now because `JdbcBasePersistenceImpl` implements both the MetaStore
interfaces and `MetricsPersistence`.
Given the current state of the JDBC-related code I'd prefer to remove this
parameter completely.
We can add it late if/when `MetricsPersistence` is isolated from MetaStore
Persistence.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2925005557
##
runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import io.quarkus.arc.DefaultBean;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
+ * single default {@link DataSource}. This implementation acts as a fallback;
downstream users can
+ * provide their own {@link DataSourceResolver} bean to implement custom
routing logic.
+ */
+@ApplicationScoped
+@DefaultBean
Review Comment:
Good idea 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2924956749
##
runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
and store types to a
+ * single default {@link DataSource}. This serves as both the production
default and the base for
+ * multi-datasource extensions.
+ */
+@ApplicationScoped
Review Comment:
Whoever introduces an alternative implementation for `DataSourceResolver`
should provide a custom producer method or annotate the preferred bean with a
higher `@Priority` than this default.
This is a common pattern in Polaris in a few other extension points.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4047041785 @Subham-KRLX : thanks for your investigation into CDI startup behaviour (above) :+1: -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2923944321
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store type. This enables
Review Comment:
Thanks for the feedback @flyingImer I completely agree. I have pushed an
update that narrows the scope of this first step to strictly cover METASTORE
routing, completely removing METRICS and EVENTS from StoreType and updating the
Javadoc to reflect this tighter scope. I also removed the unnecessary runtime
ambiguity by injecting DataSourceResolver directly instead of
Instance. Finally, I added @DefaultBean to
DefaultDataSourceResolver so downstream users can cleanly override the routing
logic without ambiguous CDI resolution errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4045943100 > As I said in the mailing thread https://lists.apache.org/thread/nokyythvrdzsfwz26hx0w5rpxrw5wjtd, I think this effort deserve more thoughts before we start coding. cc @singhpk234 @flyrain I agree that the full metrics/events routing story needs a broader design discussion! To address this exact concern (raised here and by EJ Wang), I just pushed an update to significantly narrow the scope of this PR. I have entirely removed METRICS and EVENTS—this PR is now solely establishing the DataSourceResolver SPI foundation for the METASTORE. This gives downstream users a cleaner injection point without locking in any architecture for the new tables. Does this narrower first step alleviate your concerns for this specific PR? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4045931648 > > [...] The root cause was DefaultDataSourceResolver unconditionally injecting a JDBC DataSource bean, which caused an io.quarkus.arc.InactiveBeanException when booting profiles where JDBC is disabled (like NoSQL/Mongo). > > Current code in this PR LGTM, however, I still wonder why would `DefaultDataSourceResolver` be "engaged" in non-JDBC contexts (and subsequently get the `InactiveBeanException`) 🤔 > > @Subham-KRLX : Could you explore this a bit more? > > Perhaps `DefaultDataSourceResolver` may need to be a `@Dependent` bean rather than `@ApplicationScoped` 🤔 Hi @dimas-b the root issue is Quarkus ArC's eager boot-time validation.Since DefaultDataSourceResolver is in runtime-common, it’s always discovered during boot (even in NoSQL profiles). The Quarkus JDBC extension disables the DataSource bean when JDBC is off. A direct @Inject DataSource causes Quarkus to throw an InactiveBeanException immediately at boot because the required injection point cannot be satisfied. By using @Inject Instance, we make the injection point dynamic/lazy, bypassing the strict upfront validation. (Even if the bean were @Dependent, Quarkus ArC still eagerly validates all discovered injection points by default). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
flyingImer commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2922156977
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store type. This enables
+ * isolating different workloads (e.g., entity metadata vs metrics vs events)
into different
+ * physical databases or connection pools.
+ */
+public interface DataSourceResolver {
+
Review Comment:
I wonder if this contract is getting a bit ahead of the implementation. This
PR only wires `METASTORE`, but the SPI already publishes `METRICS` and `EVENTS`
as if those routing modes were part of the current supported surface. Would it
make sense to keep the first step narrower and add new `StoreType` values only
once the corresponding paths are actually routed through this resolver?
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store type. This enables
Review Comment:
IIUC, this Javadoc is describing the longer-term shape more than what the PR
actually does today. Right now this is a metastore-routing foundation, not a
system-wide metadata/metrics/events routing layer. I wonder if tightening the
wording to match the current behavior would make the contract less confusing
for future implementors/readers.
##
runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.quarkus.common.config.jdbc;
+
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.persistence.relational.jdbc.DataSourceResolver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that route
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
flyrain commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4041356876 As I said in the mailing thread https://lists.apache.org/thread/nokyythvrdzsfwz26hx0w5rpxrw5wjtd, I think this effort deserve more thoughts before we start coding. cc @singhpk234 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4040858741 > [...] The root cause was DefaultDataSourceResolver unconditionally injecting a JDBC DataSource bean, which caused an io.quarkus.arc.InactiveBeanException when booting profiles where JDBC is disabled (like NoSQL/Mongo). Current code in this PR LGTM, however, I still wonder why would `DefaultDataSourceResolver` be "engaged" in non-JDBC contexts (and subsequently get the `InactiveBeanException`) :thinking: @Subham-KRLX : Could you explore this a bit more? Perhaps `DefaultDataSourceResolver` may need to be a `@Dependent` bean rather than `@ApplicationScoped` :thinking: -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4037108274 @dimas-b @flyrain I’ve pushed an update to fix the CI failures in the non-JDBC test suites. The root cause was DefaultDataSourceResolver unconditionally injecting a JDBC DataSource bean, which caused an io.quarkus.arc.InactiveBeanException when booting profiles where JDBC is disabled (like NoSQL/Mongo). I’ve updated the implementation to use Instance for lazy resolution, allowing the service to boot safely across all persistence modes. All CI checks should now pass cleanly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4031871427 @Subham-KRLX : the PR LGTM, but please check CI failures out :sweat_smile: -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2909138520
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store type. This enables
+ * isolating different workloads (e.g., entity metadata vs metrics vs events)
into different
+ * physical databases or connection pools.
+ */
+public interface DataSourceResolver {
+
+ /** The type of store representing the workload pattern. */
+ enum StoreType {
+METASTORE,
+METRICS,
+EVENTS
+ }
+
+ /**
+ * Resolves the DataSource for a given realm and store type.
+ *
+ * @param realmId the realm identifier
+ * @param storeType the type of store (e.g., main, metrics, events)
+ * @return the resolved DataSource
+ */
+ DataSource resolve(
+ org.apache.polaris.core.context.RealmContext realmContext, StoreType
storeType);
Review Comment:
@dimas-b I have pushed an update to resolve the CI failures. It turned out
to be a CDI injection issue with the @Identifier annotation that I have now
resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2909076964
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store
+ * type. This enables
+ * isolating different workloads (e.g., entity metadata vs metrics vs events)
+ * into different
+ * physical databases or connection pools.
+ */
+public interface DataSourceResolver {
+
+ /** The type of store representing the workload pattern. */
+ enum StoreType {
+METASTORE,
+METRICS,
+EVENTS
+ }
+
+ /**
+ * Resolves the DataSource for a given realm and store type.
+ *
+ * @param realmContext the realm context
+ * @param storeTypethe type of store (e.g., main, metrics, events)
+ * @return the resolved DataSource
+ */
+ DataSource resolve(RealmContext realmContext, StoreType storeType);
Review Comment:
@flyrain Could you clarify your concerns regarding StoreType? Are you
suggesting a more extensible approach (e.g., String identifiers) or perhaps
routing at a different layer? Happy to refine this to better fit the project's
direction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
flyrain commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2908505814
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store
+ * type. This enables
+ * isolating different workloads (e.g., entity metadata vs metrics vs events)
+ * into different
+ * physical databases or connection pools.
+ */
+public interface DataSourceResolver {
+
+ /** The type of store representing the workload pattern. */
+ enum StoreType {
+METASTORE,
+METRICS,
+EVENTS
+ }
+
+ /**
+ * Resolves the DataSource for a given realm and store type.
+ *
+ * @param realmContext the realm context
+ * @param storeTypethe type of store (e.g., main, metrics, events)
+ * @return the resolved DataSource
+ */
+ DataSource resolve(RealmContext realmContext, StoreType storeType);
Review Comment:
I'm supportive on realm, but we will need to think about the storage type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4026171372 Thanks for the approval @dimas-b. I have fixed that last import nit and have sent the proposal email to the dev list [1] for broader visibility as you suggested. Appreciate all the guidance on streamlining this. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2907431322
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store type. This enables
+ * isolating different workloads (e.g., entity metadata vs metrics vs events)
into different
+ * physical databases or connection pools.
+ */
+public interface DataSourceResolver {
+
+ /** The type of store representing the workload pattern. */
+ enum StoreType {
+METASTORE,
+METRICS,
+EVENTS
+ }
+
+ /**
+ * Resolves the DataSource for a given realm and store type.
+ *
+ * @param realmId the realm identifier
+ * @param storeType the type of store (e.g., main, metrics, events)
+ * @return the resolved DataSource
+ */
+ DataSource resolve(
+ org.apache.polaris.core.context.RealmContext realmContext, StoreType
storeType);
Review Comment:
nit: import
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2907430036
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import java.util.Set;
+import javax.sql.DataSource;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store
+ * type.
+ * This enables isolating different workloads (e.g., entity metadata vs metrics
+ * vs events)
+ * into different physical databases or connection pools.
+ */
+public interface DataSourceResolver {
+
+/** The type of store representing the workload pattern. */
+enum StoreType {
+MAIN,
+METRICS,
+EVENTS
+}
+
+/**
+ * Resolves the DataSource for a given realm and store type.
+ *
+ * @param realmId the realm identifier
+ * @param storeType the type of store (e.g., main, metrics, events)
+ * @return the resolved DataSource
+ */
+DataSource resolve(String realmId, StoreType storeType);
Review Comment:
I have applied the final round of nits: renamed MAIN to METASTORE, switched
to RealmContext in resolve()
for consistency, and fixed the @Inject field formatting. I'm also sending
an email to [email protected]
now to give this more visibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2907367098
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import java.util.Set;
+import javax.sql.DataSource;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store
+ * type.
+ * This enables isolating different workloads (e.g., entity metadata vs metrics
+ * vs events)
+ * into different physical databases or connection pools.
+ */
+public interface DataSourceResolver {
+
+/** The type of store representing the workload pattern. */
+enum StoreType {
+MAIN,
Review Comment:
nit: `MAIN` -> `METASTORE`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2907386737
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import java.util.Set;
+import javax.sql.DataSource;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store
+ * type.
+ * This enables isolating different workloads (e.g., entity metadata vs metrics
+ * vs events)
+ * into different physical databases or connection pools.
+ */
+public interface DataSourceResolver {
+
+/** The type of store representing the workload pattern. */
+enum StoreType {
+MAIN,
+METRICS,
+EVENTS
+}
+
+/**
+ * Resolves the DataSource for a given realm and store type.
+ *
+ * @param realmId the realm identifier
+ * @param storeType the type of store (e.g., main, metrics, events)
+ * @return the resolved DataSource
+ */
+DataSource resolve(String realmId, StoreType storeType);
Review Comment:
I'm thinking that we may want to use `RealmContext` instead of `String`
realm ID here... just to continue the pattern of using `RealmContext` in
pluggable components... e.g. `RealmConfigurationSource` , `TokenBucketFactory`,
etc. 🤔
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##
@@ -71,14 +72,21 @@ public class JdbcMetaStoreManagerFactory implements
MetaStoreManagerFactory {
final Map entityCacheMap = new HashMap<>();
final Map> sessionSupplierMap = new
HashMap<>();
- @Inject Clock clock;
- @Inject PolarisDiagnostics diagnostics;
- @Inject PolarisStorageIntegrationProvider storageIntegrationProvider;
- @Inject Instance dataSource;
- @Inject RelationalJdbcConfiguration relationalJdbcConfiguration;
- @Inject RealmConfig realmConfig;
-
- protected JdbcMetaStoreManagerFactory() {}
+ @Inject
+ Clock clock;
Review Comment:
nit: I wonder if the Spotless is going to complain about this 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2907368465
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import java.util.Set;
+import javax.sql.DataSource;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store
+ * type.
+ * This enables isolating different workloads (e.g., entity metadata vs metrics
+ * vs events)
+ * into different physical databases or connection pools.
+ */
+public interface DataSourceResolver {
+
+String STORE_TYPE_MAIN = "main";
Review Comment:
I have refactored the store types into the StoreType enum as suggested. This
makes the resolution logic much cleaner and more type-safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
Subham-KRLX commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4026049160 @dimas-b thanks for the review I have streamlined the implementation based on your feedback by refactoring StoreType into an enum within DataSourceResolver and removing the getAllUniqueDataSources() method to keep the interface focused strictly on runtime resolution. Additionally I moved DefaultDataSourceResolver to the runtime-common module to better decouple the persistence layer from resolution logic and simplified the implementation by injecting a plain DataSource instead of an Instance. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on code in PR #3960:
URL: https://github.com/apache/polaris/pull/3960#discussion_r2907236152
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DataSourceResolver.java:
##
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import java.util.Set;
+import javax.sql.DataSource;
+
+/**
+ * Service to resolve the correct {@link DataSource} for a given realm and
store
+ * type.
+ * This enables isolating different workloads (e.g., entity metadata vs metrics
+ * vs events)
+ * into different physical databases or connection pools.
+ */
+public interface DataSourceResolver {
+
+String STORE_TYPE_MAIN = "main";
Review Comment:
Maybe an `enum`?
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import java.util.HashSet;
+import java.util.Set;
+import javax.sql.DataSource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default implementation of {@link DataSourceResolver} that routes all realms
+ * and store types to a
+ * single default {@link DataSource}. This serves as both the production
default
+ * and the base for
+ * multi-datasource extensions.
+ *
+ *
+ * To enable per-realm or per-store datasource routing, this class can be
+ * extended or replaced
+ * with a custom implementation that resolves named datasources based on
+ * configuration.
+ */
+@ApplicationScoped
+public class DefaultDataSourceResolver implements DataSourceResolver {
+
+private static final Logger LOGGER =
LoggerFactory.getLogger(DefaultDataSourceResolver.class);
+
+private final Instance defaultDataSource;
Review Comment:
Does this have to be an `Instance`? Why not use the plain `DataSource` as
type here?
##
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DefaultDataSourceResolver.java:
##
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Instance;
+import jakarta.inject.Inject;
+import java.util.HashSet;
+import java.util.Set;
+import javax.sql.DataSource;
+import org.slf4j.Logger;
+import org.
Re: [PR] Introduce DataSourceResolver for multi-datasource support in JDBC per… [polaris]
dimas-b commented on PR #3960: URL: https://github.com/apache/polaris/pull/3960#issuecomment-4025904214 @Subham-KRLX : thanks for the proposal... I'll review presently, but please fix conflicts to allow CI to run. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
