Implement #33646 (#33865)

* Implement #33646

* More dox

* Seconds, not milliseconds

* Respect env var values

src/metabase/driver/sql_jdbc/connection.clj

@@ -3,7 +3,6 @@
   multimethods for SQL JDBC drivers."
   (:require
    [clojure.java.jdbc :as jdbc]
-   [metabase.config :as config]
    [metabase.connection-pool :as connection-pool]
    [metabase.db.connection :as mdb.connection]
    [metabase.driver :as driver]
@@ -11,6 +10,8 @@
    [metabase.lib.metadata.jvm :as lib.metadata.jvm]
    [metabase.models.database :refer [Database]]
    [metabase.models.interface :as mi]
+   [metabase.models.setting :as setting]
+   [metabase.query-processor.context.default :as context.default]
    [metabase.query-processor.store :as qp.store]
    [metabase.util :as u]
    [metabase.util.i18n :refer [trs tru]]
@@ -81,6 +82,24 @@
             :catalog)
    details))
 
+(setting/defsetting jdbc-data-warehouse-max-connection-pool-size
+  "Maximum size of the c3p0 connection pool."
+  :visibility :internal
+  :type :integer
+  :default 15)
+
+(setting/defsetting jdbc-data-warehouse-unreturned-connection-timeout-seconds
+  "Kill connections if they are unreturned after this amount of time. In theory this should not be needed because the QP
+  will kill connections that time out, but in practice it seems that connections disappear into the ether every once
+  in a while; rather than exhaust the connection pool, let's be extra safe. This should be the same as the query
+  timeout in [[metabase.query-processor.context.default/query-timeout-ms]] by default."
+  :visibility :internal
+  :type       :integer
+  :getter     (fn []
+                (or (setting/get-value-of-type :integer :jdbc-data-warehouse-unreturned-connection-timeout-seconds)
+                    (long (/ context.default/query-timeout-ms 1000))))
+  :setter     :none)
+
 (defmethod data-warehouse-connection-pool-properties :default
   [driver database]
   { ;; only fetch one new connection at a time, rather than batching fetches (default = 3 at a time). This is done in
@@ -90,8 +109,7 @@
    "maxIdleTime"                  (* 3 60 60) ; 3 hours
    "minPoolSize"                  1
    "initialPoolSize"              1
-   "maxPoolSize"                  (or (config/config-int :mb-jdbc-data-warehouse-max-connection-pool-size)
-                                      15)
+   "maxPoolSize"                  (jdbc-data-warehouse-max-connection-pool-size)
    ;; [From dox] If true, an operation will be performed at every connection checkout to verify that the connection is
    ;; valid. [...] ;; Testing Connections in checkout is the simplest and most reliable form of Connection testing,
    ;; but for better performance, consider verifying connections periodically using `idleConnectionTestPeriod`. [...]
@@ -116,6 +134,10 @@
    ;;
    ;; Kill idle connections above the minPoolSize after 5 minutes.
    "maxIdleTimeExcessConnections" (* 5 60)
+   ;; kill connections after this amount of time if they haven't been returned -- this should be the same as the query
+   ;; timeout. This theoretically shouldn't happen since the QP should kill things after a certain timeout but it's
+   ;; better to be safe than sorry -- it seems like in practice some connections disappear into the ether
+   "unreturnedConnectionTimeout"  (jdbc-data-warehouse-unreturned-connection-timeout-seconds)
    ;; Set the data source name so that the c3p0 JMX bean has a useful identifier, which incorporates the DB ID, driver,
    ;; and name from the details
    "dataSourceName"               (format "db-%d-%s-%s"

test/metabase/driver/sql_jdbc/connection_test.clj

@@ -76,7 +76,7 @@
                (testing "the pool has been destroyed"
                  (is @destroyed?))))))))))
 
-(deftest c3p0-datasource-name-test
+(deftest ^:parallel c3p0-datasource-name-test
   (mt/test-drivers (sql-jdbc.tu/sql-jdbc-drivers)
     (testing "The dataSourceName c3p0 property is set properly for a database"
       (let [db         (mt/db)
@@ -87,7 +87,7 @@
         ;; ensure that, for any sql-jdbc driver anyway, we found *some* DB name to use in this String
         (is (not= db-nm "null"))))))
 
-(deftest same-connection-details-result-in-equal-specs-test
+(deftest ^:parallel same-connection-details-result-in-equal-specs-test
   (testing "Two JDBC specs created with the same details must be considered equal for the connection pool cache to work correctly"
     ;; this is only really a concern for drivers like Spark SQL that create custom DataSources instead of plain details
     ;; maps -- those DataSources need to be considered equal based on the connection string/properties
@@ -201,3 +201,14 @@
         (is (= first-pool second-pool))
         (is (= ::audit-db-not-in-cache!
                (get @#'sql-jdbc.conn/database-id->connection-pool audit-db-id ::audit-db-not-in-cache!)))))))
+
+(deftest ^:parallel include-unreturned-connection-timeout-test
+  (testing "We should be setting unreturnedConnectionTimeout; it should be the same as the query timeout (#33646)"
+    (is (=? {"unreturnedConnectionTimeout" integer?}
+            (sql-jdbc.conn/data-warehouse-connection-pool-properties :h2 (mt/db))))))
+
+(deftest unreturned-connection-timeout-test
+  (testing "We should be able to set jdbc-data-warehouse-unreturned-connection-timeout-seconds via env var (#33646)"
+    (mt/with-temp-env-var-value [mb-jdbc-data-warehouse-unreturned-connection-timeout-seconds "20"]
+      (is (= 20
+             (sql-jdbc.conn/jdbc-data-warehouse-unreturned-connection-timeout-seconds))))))