From a610b92bda2c8fcc4a54f7a60e8428150fb498a6 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Mon, 18 Oct 2021 14:26:31 -0700
Subject: [PATCH] Consolidate DB connection timeouts; raise default to 10
seconds (#18541)
---
.../mongo/src/metabase/driver/mongo/util.clj | 45 ++++++++-----------
src/metabase/driver/util.clj | 21 ++++++---
2 files changed, 33 insertions(+), 33 deletions(-)
diff --git a/modules/drivers/mongo/src/metabase/driver/mongo/util.clj b/modules/drivers/mongo/src/metabase/driver/mongo/util.clj
index cb608cc20ee..9460866e10e 100644
--- a/modules/drivers/mongo/src/metabase/driver/mongo/util.clj
+++ b/modules/drivers/mongo/src/metabase/driver/mongo/util.clj
@@ -13,14 +13,6 @@
[toucan.db :as db])
(:import [com.mongodb MongoClient MongoClientOptions MongoClientOptions$Builder MongoClientURI]))
-(def ^:const ^:private connection-timeout-ms
- "Number of milliseconds to wait when attempting to establish a Mongo connection. By default, Monger uses a 10-second
- timeout, which means `can/connect?` can take forever, especially when called with bad details. This translates to
- our tests taking longer and the DB setup API endpoints seeming sluggish.
-
- Don't set the timeout too low -- I've have Circle fail when the timeout was 1000ms on *one* occasion."
- 3000)
-
(def ^:dynamic ^com.mongodb.DB *mongo-connection*
"Connection to a Mongo database. Bound by top-level `with-mongo-connection` so it may be reused within its body."
nil)
@@ -35,10 +27,10 @@
;; these options for us and return a `MongoClientOptions` like we'd prefer. Code below:
(defn- client-options-for-url-params
- "Return an instance of `MongoClientOptions` from a URL-PARAMS string, e.g.
+ "Return an instance of `MongoClientOptions` from a `url-params` string, e.g.
- (client-options-for-url-params \"readPreference=nearest\")
- ;; -> #MongoClientOptions{readPreference=nearest, ...}"
+ (client-options-for-url-params \"readPreference=nearest\")
+ ;; -> #MongoClientOptions{readPreference=nearest, ...}"
^MongoClientOptions [^String url-params]
(when (seq url-params)
;; just make a fake connection string to tack the URL params on to. We can use that to have the Mongo lib
@@ -65,8 +57,8 @@
(let [client-options (-> (client-options-for-url-params additional-options)
client-options->builder
(.description config/mb-app-id-string)
- (.connectTimeout connection-timeout-ms)
- (.serverSelectionTimeout connection-timeout-ms)
+ (.connectTimeout (driver.u/db-connection-timeout-ms))
+ (.serverSelectionTimeout (driver.u/db-connection-timeout-ms))
(.sslEnabled ssl?))]
(if (not (str/blank? ssl-cert))
(-> client-options
@@ -211,9 +203,9 @@
(let [mongo-client (mg/connect-via-uri conn-string)]
[(:conn mongo-client) (:db mongo-client)]))
-(defn -with-mongo-connection
- "Run `f` with a new connection (bound to `*mongo-connection*`) to `database`. Don't use this directly; use
- `with-mongo-connection`."
+(defn do-with-mongo-connection
+ "Run `f` with a new connection (bound to [[*mongo-connection*]]) to `database`. Don't use this directly; use
+ [[with-mongo-connection]]."
[f database]
(let [details (database->details database)]
(ssh/with-ssh-tunnel [details-with-tunnel details]
@@ -227,24 +219,23 @@
(mg/disconnect mongo-client)
(log/debug (u/format-color 'cyan (trs "Closed MongoDB connection.")))))))))
-
(defmacro with-mongo-connection
"Open a new MongoDB connection to ``database-or-connection-string`, bind connection to `binding`, execute `body`, and
- close the connection. The DB connection is re-used by subsequent calls to `with-mongo-connection` within
- `body`. (We're smart about it: `database` isn't even evaluated if `*mongo-connection*` is already bound.)
+ close the connection. The DB connection is re-used by subsequent calls to [[with-mongo-connection]] within
+ `body`. (We're smart about it: `database` isn't even evaluated if [[*mongo-connection*]] is already bound.)
- ;; delay isn't derefed if *mongo-connection* is already bound
- (with-mongo-connection [^com.mongodb.DB conn @(:db (sel :one Table ...))]
- ...)
+ ;; delay isn't derefed if *mongo-connection* is already bound
+ (with-mongo-connection [^com.mongodb.DB conn @(:db (sel :one Table ...))]
+ ...)
- ;; You can use a string instead of a Database
- (with-mongo-connection [^com.mongodb.DB conn \"mongodb://127.0.0.1:27017/test\"]
- ...)
+ ;; You can use a string instead of a Database
+ (with-mongo-connection [^com.mongodb.DB conn \"mongodb://127.0.0.1:27017/test\"]
+ ...)
- DATABASE-OR-CONNECTION-STRING can also optionally be the connection details map on its own."
+ `database-or-connection-string` can also optionally be the connection details map on its own."
[[binding database] & body]
`(let [f# (fn [~binding]
~@body)]
(if *mongo-connection*
(f# *mongo-connection*)
- (-with-mongo-connection f# ~database))))
+ (do-with-mongo-connection f# ~database))))
diff --git a/src/metabase/driver/util.clj b/src/metabase/driver/util.clj
index 2cbcfdcdb7a..1b49e7ff60e 100644
--- a/src/metabase/driver/util.clj
+++ b/src/metabase/driver/util.clj
@@ -4,6 +4,7 @@
[clojure.tools.logging :as log]
[metabase.config :as config]
[metabase.driver :as driver]
+ [metabase.models.setting :refer [defsetting]]
[metabase.util :as u]
[metabase.util.i18n :refer [trs]]
[toucan.db :as db])
@@ -13,11 +14,19 @@
javax.net.SocketFactory
[javax.net.ssl SSLContext TrustManagerFactory X509TrustManager]))
-(def ^:private can-connect-timeout-ms
- "Consider `can-connect?`/`can-connect-with-details?` to have failed after this many milliseconds.
- By default, this is 5 seconds. You can configure this value by setting the env var `MB_DB_CONNECTION_TIMEOUT_MS`."
- (or (config/config-int :mb-db-connection-timeout-ms)
- 5000))
+;; This is normally set via the env var `MB_DB_CONNECTION_TIMEOUT_MS`
+(defsetting db-connection-timeout-ms
+ "Consider [[metabase.driver/can-connect?]] / [[can-connect-with-details?]] to have failed if they were not able to
+ successfully connect after this many milliseconds. By default, this is 10 seconds."
+ :visibility :internal
+ :type :integer
+ ;; for TESTS use a timeout time of 3 seconds. This is because we have some tests that check whether
+ ;; [[driver/can-connect?]] is failing when it should, and we don't want them waiting 10 seconds to fail.
+ ;;
+ ;; Don't set the timeout too low -- I've have Circle fail when the timeout was 1000ms on *one* occasion.
+ :default (if config/is-test?
+ 3000
+ 10000))
(defn can-connect-with-details?
"Check whether we can connect to a database with `driver` and `details-map` and perform a basic query such as `SELECT
@@ -30,7 +39,7 @@
{:pre [(keyword? driver) (map? details-map)]}
(if throw-exceptions
(try
- (u/with-timeout can-connect-timeout-ms
+ (u/with-timeout (db-connection-timeout-ms)
(driver/can-connect? driver details-map))
;; actually if we are going to `throw-exceptions` we'll rethrow the original but attempt to humanize the message
;; first
--
GitLab