Copilot commented on code in PR #4812: URL: https://github.com/apache/polaris/pull/4812#discussion_r3430071412
########## runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/JdbcProducers.java: ########## @@ -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.quarkus.common.config.jdbc; + +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.enterprise.inject.Any; +import jakarta.enterprise.inject.Instance; +import jakarta.enterprise.inject.Produces; +import jakarta.enterprise.inject.literal.NamedLiteral; +import javax.sql.DataSource; +import org.apache.polaris.persistence.relational.jdbc.DatasourceOperations; + +public class JdbcProducers { + + @Produces + @ApplicationScoped + public DatasourceOperations produceDatasourceOperations( + @Any Instance<DataSource> dataSources, + QuarkusRelationalJdbcConfiguration relationalJdbcConfiguration) { + String dataSourceName = relationalJdbcConfiguration.dataSource(); + Instance<DataSource> dataSourceInstance = dataSources.select(NamedLiteral.of(dataSourceName)); + if (dataSourceInstance.isUnsatisfied()) { + throw new IllegalStateException("datasource " + dataSourceName + " not found"); + } Review Comment: Quarkus named datasources are qualified with `@io.quarkus.agroal.DataSource(\"name\")` rather than `@Named(\"name\")`. Selecting with `NamedLiteral` will likely leave the instance unsatisfied at runtime. Use the Agroal datasource qualifier literal (e.g., `io.quarkus.agroal.DataSource.DataSourceLiteral.of(dataSourceName)`) for selection, and align the injection sites accordingly. ########## runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/JdbcProducers.java: ########## @@ -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.quarkus.common.config.jdbc; + +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.enterprise.inject.Any; +import jakarta.enterprise.inject.Instance; +import jakarta.enterprise.inject.Produces; +import jakarta.enterprise.inject.literal.NamedLiteral; +import javax.sql.DataSource; +import org.apache.polaris.persistence.relational.jdbc.DatasourceOperations; + +public class JdbcProducers { + + @Produces + @ApplicationScoped + public DatasourceOperations produceDatasourceOperations( + @Any Instance<DataSource> dataSources, + QuarkusRelationalJdbcConfiguration relationalJdbcConfiguration) { + String dataSourceName = relationalJdbcConfiguration.dataSource(); + Instance<DataSource> dataSourceInstance = dataSources.select(NamedLiteral.of(dataSourceName)); + if (dataSourceInstance.isUnsatisfied()) { + throw new IllegalStateException("datasource " + dataSourceName + " not found"); Review Comment: This exception message is not very actionable for operators. Consider including the controlling property name (`polaris.persistence.relational.jdbc.datasource`) and a hint that the expected bean must be a Quarkus named datasource (and ideally list the available datasource names when unsatisfied) to speed up debugging misconfiguration. ########## runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/QuarkusRelationalJdbcConfiguration.java: ########## @@ -19,7 +19,34 @@ package org.apache.polaris.quarkus.common.config.jdbc; import io.smallrye.config.ConfigMapping; +import io.smallrye.config.WithName; +import java.util.Optional; import org.apache.polaris.persistence.relational.jdbc.RelationalJdbcConfiguration; @ConfigMapping(prefix = "polaris.persistence.relational.jdbc") -public interface QuarkusRelationalJdbcConfiguration extends RelationalJdbcConfiguration {} +public interface QuarkusRelationalJdbcConfiguration extends RelationalJdbcConfiguration { + + /** The maximum number of retries before giving up the operation. */ + @Override + Optional<Integer> maxRetries(); + + /** The maximum retry duration in milliseconds. */ + @Override + Optional<Long> maxDurationInMs(); + + /** The initial retry delay. */ + @Override + Optional<Long> initialDelayInMs(); + + /** + * Explicitly configured database type. If not specified, the database type will be inferred from + * the JDBC connection metadata. Supported values: "postgresql", "cockroachdb", "h2" + */ + @Override + Optional<String> databaseType(); + + /** The datasource name to use. Required. */ + @Override + @WithName("datasource") + String dataSource(); Review Comment: `dataSource()` is marked as required (non-Optional) in the config mapping, but `DataSourceActivator` also defines a `DEFAULT_DATASOURCE_NAME` fallback when the property is missing. To keep behavior consistent, either (a) make the mapping provide the same default (e.g., via a default value annotation) or (b) remove the fallback from the activator and fail fast when the property is absent. ########## runtime/service/src/test/java/org/apache/polaris/service/events/listeners/inmemory/InMemoryBufferEventListenerTestBase.java: ########## @@ -59,7 +60,11 @@ abstract class InMemoryBufferEventListenerTestBase { @SuppressWarnings("CdiInjectionPointsInspection") EventLoopGroup eventLoopGroup; - @Inject Instance<DataSource> dataSource; + + @Inject + @Named("h2") + Instance<DataSource> dataSource; Review Comment: This injection uses `@jakarta.inject.Named(\"h2\")`, but Quarkus named datasources are typically injected with `@io.quarkus.agroal.DataSource(\"h2\")` (the Agroal qualifier), not `@Named`. As written, CDI may not resolve the datasource bean. Switch the qualifier to the Quarkus Agroal datasource qualifier to match how Quarkus exposes named `DataSource` beans. ########## runtime/common/src/main/java/org/apache/polaris/quarkus/common/config/jdbc/DataSourceActivator.java: ########## @@ -0,0 +1,122 @@ +/* + * 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.smallrye.config.ConfigSourceInterceptor; +import io.smallrye.config.ConfigSourceInterceptorContext; +import io.smallrye.config.ConfigValue; +import io.smallrye.config.ConfigValue.ConfigValueBuilder; +import io.smallrye.config.Priorities; +import jakarta.annotation.Priority; + +/** + * Activates a data source based on the current Polaris configuration under {@link + * #POLARIS_DATASOURCE_NAME_PROPERTY}, and deactivates all other data sources. + * + * <p>If the persistence type is not JDBC, all data sources are deactivated. + */ +@Priority(Priorities.LIBRARY + 400) +public class DataSourceActivator implements ConfigSourceInterceptor { Review Comment: New configuration behavior is introduced here (runtime activation/deactivation of named datasources), but there’s no corresponding automated test in this diff. Add unit/integration tests that assert: (1) only the selected `quarkus.datasource.<name>.active` becomes `true` when `polaris.persistence.type=relational-jdbc`, (2) all datasources are inactive for other persistence types, and (3) the default datasource (`quarkus.datasource.active`) remains disabled. ########## persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStorePostgresIT.java: ########## @@ -82,6 +82,11 @@ public Optional<Long> initialDelayInMs() { public Optional<String> databaseType() { return Optional.empty(); } + + @Override + public String dataSource() { + return "postgres"; + } Review Comment: The configured datasource name `\"postgres\"` is inconsistent with the rest of the PR/docs (which standardize on `\"postgresql\"`). Even if this implementation is mostly unused in this test, keeping datasource names consistent reduces confusion and avoids future breakage if `dataSource()` starts being used in shared code paths. -- 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]
