Skip to content
Closed
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
136e381
Introduce DataSourceResolver for multi-datasource support in JDBC per…
Subham-KRLX Mar 9, 2026
207aa4c
Address review comments on DataSourceResolver PR
Subham-KRLX Mar 9, 2026
e024bea
Final review nits on DataSourceResolver PR
Subham-KRLX Mar 9, 2026
96e7ff0
Address final nit: use import for RealmContext
Subham-KRLX Mar 9, 2026
9a07e6e
Final formatting and naming consistency cleanup
Subham-KRLX Mar 10, 2026
75e4096
Fix InactiveBeanException in non-JDBC profiles via lazy DataSource re…
Subham-KRLX Mar 11, 2026
fccb254
Address review comments: tighten scope, use plain injection and @Defa…
Subham-KRLX Mar 12, 2026
9c22635
Address review comments: tighten scope, use @Identifier and producer …
Subham-KRLX Mar 13, 2026
1e00328
Address final nits: Rename identifier to default and move config to J…
Subham-KRLX Mar 13, 2026
493c386
Refine DataSourceResolver configuration wiring per review
Subham-KRLX Mar 16, 2026
6311fbe
Address final CDI producer wiring nits
Subham-KRLX Mar 18, 2026
a36e745
Reinstate StoreType, update config docs, and finalize CDI wiring
Subham-KRLX Mar 23, 2026
118e046
Address review feedback: Resolve separate DataSources for METASTORE, …
Subham-KRLX Mar 24, 2026
dc8ffda
Address further review feedback: Revert unrelated changes, move CDI p…
Subham-KRLX Mar 25, 2026
7eb2304
Finalize PR #3960: split v4 DDL, fix license headers, and move CDI pr…
Subham-KRLX Mar 31, 2026
7ceaa69
Introduce DataSourceResolver for multi-datasource support in JDBC per…
Subham-KRLX Apr 1, 2026
9df42be
Refine Multi-DataSource support: selective bootstrap, independent ver…
Subham-KRLX Apr 2, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ if (System.getProperty("idea.sync.active").toBoolean()) {
eclipse { project { name = ideName } }

tasks.named<RatTask>("rat").configure {
mustRunAfter(":polaris-config-docs-site:copyConfigSectionsToSite")
// Gradle
excludes.add("**/build/**")
excludes.add("gradle/wrapper/gradle-wrapper*")
Expand Down Expand Up @@ -147,6 +148,9 @@ tasks.named<RatTask>("rat").configure {

// Rat can't scan binary images
excludes.add("**/*.png")

// Auto-generated site content (copied from docs by copyConfigSectionsToSite)
excludes.add("site/content/**")
}

tasks.register<Exec>("buildPythonClient") {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* 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. Note: Currently this is
* implemented as a foundation for metastore routing.
*/
public interface DataSourceResolver {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

/** 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 storeType the type of store
* @return the resolved DataSource
*/
DataSource resolve(RealmContext realmContext, StoreType storeType);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm supportive on realm, but we will need to think about the storage type.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Contributor

@dimas-b dimas-b Mar 23, 2026

Choose a reason for hiding this comment

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

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.

}
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,16 @@ public static DatabaseType inferFromConnection(
* caller.
*/
public InputStream openInitScriptResource(int schemaVersion) {
return openInitScriptResource(schemaVersion, null);
}

/**
* Open an InputStream that contains data from an init script for a specific {@link
* DataSourceResolver.StoreType}. If a specialized script (e.g., schema-v4-metrics.sql) is not
* found, it falls back to the main script (schema-v4.sql).
*/
public InputStream openInitScriptResource(
int schemaVersion, DataSourceResolver.StoreType storeType) {
// Validate schema version is within acceptable range for this database type
int latestVersion = getLatestSchemaVersion();
if (schemaVersion <= 0 || schemaVersion > latestVersion) {
Expand All @@ -158,10 +168,21 @@ public InputStream openInitScriptResource(int schemaVersion) {
schemaVersion, this, latestVersion));
}

ClassLoader classLoader = DatasourceOperations.class.getClassLoader();
if (storeType != null) {
String specializedResourceName =
String.format(
"%s/schema-v%d-%s.sql",
this.getDisplayName(), schemaVersion, storeType.name().toLowerCase(Locale.ROOT));
InputStream specializedStream = classLoader.getResourceAsStream(specializedResourceName);
if (specializedStream != null) {
return specializedStream;
}
}

final String resourceName =
String.format("%s/schema-v%d.sql", this.getDisplayName(), schemaVersion);

ClassLoader classLoader = DatasourceOperations.class.getClassLoader();
InputStream stream = classLoader.getResourceAsStream(resourceName);
if (stream == null) {
throw new IllegalStateException(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* 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 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.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Default implementation of {@link DataSourceResolver} that routes all realms to a single default
* {@link DataSource}.
*/
@ApplicationScoped
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IIUC, the current extension story is still a bit fragile here. This default resolver is just an unqualified @ApplicationScoped bean, while the factory later resolves DataSourceResolver via Instance<DataSourceResolver>.get(). Would a custom unqualified resolver now turn this into ambiguous CDI resolution rather than a clean override? If this is meant to be a public SPI, I wonder if we should make the replacement model explicit up front.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd rather use @Identifier.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@Identifier("default")
public class DefaultDataSourceResolver implements DataSourceResolver {
Comment thread
dimas-b marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why this class was placed in runtime/common. This creates a strange inheritance. This class could live in polaris-relational-jdbc alongside its interface. That module already has application-scoped beans. (The only thing it doesn't have is the @DefaultBean annotation – see my other comment.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

@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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 😅

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Only "general principles" kind of reason 🙂

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 🤔 🤷 In my personal experience, I find that it is much easier to include another module into a build than struggle with excluding intruding dependencies 😅

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.


private static final Logger LOGGER = LoggerFactory.getLogger(DefaultDataSourceResolver.class);

private final Instance<DataSource> defaultDataSource;

@Inject
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You are probably missing the @Any annotation here.

But more broadly: if this impl can only handle one datasource, the default one, why not just inject it directly?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 : #3960 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK makes sense 👍

public DefaultDataSourceResolver(@Any Instance<DataSource> defaultDataSource) {
this.defaultDataSource = defaultDataSource;
}

@Override
public DataSource resolve(RealmContext realmContext, StoreType storeType) {
LOGGER.debug(
"Using default DataSource for realm '{}' and store '{}'",
realmContext.getRealmIdentifier(),
storeType);
return defaultDataSource.get();
}
}
Loading
Loading