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 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: Hmm I'm torn on `@DefaultBean`. It's not a bad idea per se, but 1) it's a Quarkus-specific, non-standard CDI extension and 2) in Polaris so far we've been using `@Identifier` instead, and at runtime we would select the bean via a producer method + a configuration option. ########## 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: I'd rather use `@Identifier`. ########## 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 { + + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultDataSourceResolver.class); + + private final Instance<DataSource> defaultDataSource; + + @Inject Review Comment: 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? ########## 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: 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.) -- 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]
