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<String> 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 Instance<DataSourceResolver> dataSourceResolvers) {
+ String type = jdbcConfig.dataSourceResolverType();
+ return dataSourceResolvers.select(Identifier.Literal.of(type)).get();
Review Comment:
It looks like we're leaning towards moving CDI/Quarkus-related code into the
`polaris-relational-jdbc` module.
Let's move this produce there too. This way the more general-purpose
`runtime/service` code can remain free from JDBC-specific logic and
dependencies.
--
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]
