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 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: 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. ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java: ########## @@ -72,10 +72,15 @@ public class JdbcMetaStoreManagerFactory implements MetaStoreManagerFactory { final Map<String, Supplier<BasePersistence>> sessionSupplierMap = new HashMap<>(); @Inject Clock clock; + @Inject PolarisDiagnostics diagnostics; + @Inject PolarisStorageIntegrationProvider storageIntegrationProvider; - @Inject Instance<DataSource> dataSource; + + @Inject Instance<DataSourceResolver> dataSourceResolver; Review Comment: Why is this injected as `Instance<DataSourceResolver>` instead of a plain `DataSourceResolver`? In the current code we unconditionally call `.get()`, so it seems we are paying the runtime ambiguity cost without really using dynamic selection. Unless we expect multiple qualified resolvers here, would direct injection give us a cleaner and safer contract? -- 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]
