dimas-b commented on code in PR #1371:
URL: https://github.com/apache/polaris/pull/1371#discussion_r2047785450
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -171,6 +181,20 @@ public boolean isAlreadyExistsException(SQLException e) {
}
private Connection borrowConnection() throws SQLException {
- return datasource.getConnection();
+ Connection connection = datasource.getConnection();
+ if (connection
+ .getMetaData()
+ .getDatabaseProductName()
+ .toLowerCase(Locale.ROOT)
+ .equals("postgresql")) {
Review Comment:
Can we avoid execution conditional on the database name? At least could we
refactor the code to use sub-classes instead of `if`s?
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -171,6 +181,20 @@ public boolean isAlreadyExistsException(SQLException e) {
}
private Connection borrowConnection() throws SQLException {
- return datasource.getConnection();
+ Connection connection = datasource.getConnection();
+ if (connection
+ .getMetaData()
Review Comment:
Isn't it expensive to do this on every `borrowConnection()`?
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -45,14 +46,15 @@ public class JdbcMetaStoreManagerFactory extends
LocalPolarisMetaStoreManagerFac
// TODO: Pending discussion of if we should have one Database per realm or 1
schema per realm
// or realm should be a primary key on all the tables.
- @Inject DataSource dataSource;
+ @Inject Instance<DataSource> dataSource;
Review Comment:
Why do we need `Instance` here?
In any case, custom CDI-related resolvers should probably be in the
Quarkus-specific modules.
##########
quarkus/admin/src/test/java/org/apache/polaris/admintool/relational/jdbc/RelationalJdbcProfile.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.admintool.relational.jdbc;
+
+import static
org.apache.polaris.admintool.PostgresTestResourceLifecycleManager.INIT_SCRIPT;
+
+import io.quarkus.test.junit.QuarkusTestProfile;
+import java.util.List;
+import java.util.Map;
+import org.apache.polaris.admintool.PostgresRelationalJdbcLifeCycleManagement;
+
+public class RelationalJdbcProfile implements QuarkusTestProfile {
+ @Override
+ public Map<String, String> getConfigOverrides() {
+ return Map.of();
+ }
+
+ @Override
+ public List<TestResourceEntry> testResources() {
+ return List.of(
+ new TestResourceEntry(
+ PostgresRelationalJdbcLifeCycleManagement.class,
+ Map.of(INIT_SCRIPT,
"org/apache/polaris/admintool/relational-jdbc/init.sql")));
Review Comment:
How can this work in user's env. when we do not know the database type
beforehand?
If this is only for testing, I believe it would be nicer to embed the SQL
into java code (e.g. as `""""` strings).
--
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]
