Re: [PR] Support Runtime JDBC Driver Loading and Dynamic Datasource Creation [polaris]

["dimas-b (via GitHub)" <gi...@apache.org>]

dimas-b commented on code in PR #4984:
URL: https://github.com/apache/polaris/pull/4984#discussion_r3532357359


##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -69,14 +69,23 @@ public class DatasourceOperations {
 
   private final DataSource datasource;
   private final RelationalJdbcConfiguration relationalJdbcConfiguration;
+  private final boolean closeDataSourceOnClose;
   private final DatabaseType databaseType;
 
   private static final Random random = new Random();
 
   public DatasourceOperations(
       DataSource datasource, RelationalJdbcConfiguration relationalJdbcConfiguration) {
+    this(datasource, relationalJdbcConfiguration, false);
+  }
+
+  DatasourceOperations(
+      DataSource datasource,
+      RelationalJdbcConfiguration relationalJdbcConfiguration,
+      boolean closeDataSourceOnClose) {

Review Comment:
   `closeDataSourceOnClose` is awkward to read. How about `assumeDataSourceOwnership`?



##########
site/content/in-dev/unreleased/metastores/relational-jdbc.md:
##########
@@ -22,12 +22,18 @@ type: docs
 weight: 100
 ---
 
-This implementation leverages Quarkus for datasource management and supports configuration through
-environment variables or JVM -D flags at startup. For more information, refer to the [Quarkus configuration reference](https://quarkus.io/guides/config-reference#env-file).
+The Relational JDBC metastore supports PostgreSQL, CockroachDB, and H2. H2 is intended for testing
+and development only.
 
-We have 2 options for configuring the persistence backend:
+Polaris can use either a Quarkus-managed datasource or a Polaris-managed JDBC connection pool. When
+`polaris.persistence.relational.jdbc.jdbc-url` is set, Polaris creates the JDBC pool directly;

Review Comment:
   The only advantage of doing this, that I can see, realizes when Polaris is able to load the JDBC driver dynamically. However, I could not find any provisions for dynamic loading in this PR. Did I miss that? 🤔 
   
   If repackaging at the binary level is assumed, a Quakus build can be performed at that stage to make the DataSource available via Quarkus-backed CDI.



##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -98,6 +107,21 @@ DatabaseType getDatabaseType() {
     return databaseType;
   }
 
+  boolean ownsDataSource() {
+    return closeDataSourceOnClose;
+  }
+
+  @Override
+  public void close() {

Review Comment:
   When is this `close()` method called?



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org