JdbcAccountPatchReviewStore: Add configurable settings to datasource

So far, the datasource used to connect to the AccountPatchReviewStore
was using hard coded values for the pool connection settings. In high
traffic servers, those default values cause a bottleneck as the
transactions start to be queued when the max number of connections is
reached.

Add the possibility of specifying those values in the gerrit
configuration so they can be tailored to diverse needs.

Change-Id: I7d0cd1e2fe02cea3c5f78ef90398e9ac670a9a93

Documentation/config-gerrit.txt

@@ -42,6 +42,53 @@ otherwise gerrit's init will remove the table.
   url = jdbc:postgresql://<host>:<port>/<db_name>?user=<user>&password=<password>
 ----
 
+[[accountPatchReviewDb.poolLimit]]accountPatchReviewDb.poolLimit::
++
+Maximum number of open database connections.  If the server needs
+more than this number, request processing threads will wait up
+to <<accountPatchReviewDb.poolMaxWait, poolMaxWait>> seconds for a
+connection to be released before they abort with an exception.
+This limit must be several units higher than the total number of
+httpd and sshd threads as some request processing code paths may
+need multiple connections.
++
+Default is <<sshd.threads, sshd.threads>>
+ + <<httpd.maxThreads, httpd.maxThreads>> + 2.
++
+
+[[accountPatchReviewDb.poolMinIdle]]database.poolMinIdle::
++
+Minimum number of connections to keep idle in the pool.
+Default is 4.
++
+
+[[accountPatchReviewDb.poolMaxIdle]]accountPatchReviewDb.poolMaxIdle::
++
+Maximum number of connections to keep idle in the pool.  If there
+are more idle connections, connections will be closed instead of
+being returned back to the pool.
+Default is min(<<accountPatchReviewDb.poolLimit, accountPatchReviewDb.poolLimit>>, 16).
++
+
+[[accountPatchReviewDb.poolMaxWait]]accountPatchReviewDb.poolMaxWait::
++
+Maximum amount of time a request processing thread will wait to
+acquire a database connection from the pool.  If no connection is
+released within this time period, the processing thread will abort
+its current operations and return an error to the client.
+Values should use common unit suffixes to express their setting:
++
+* ms, milliseconds
+* s, sec, second, seconds
+* m, min, minute, minutes
+* h, hr, hour, hours
+
++
+--
+If a unit suffix is not specified, `milliseconds` is assumed.
+
+Default is `30 seconds`.
+
 [[accounts]]
 === Section accounts
 

gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateAccountPatchReviewDb.java

@@ -19,6 +19,7 @@ import com.google.common.base.Strings;
 import com.google.gerrit.pgm.util.SiteProgram;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.config.ThreadSettingsConfig;
 import com.google.gerrit.server.schema.DataSourceProvider;
 import com.google.gerrit.server.schema.JdbcAccountPatchReviewStore;
 import com.google.inject.Injector;
@@ -47,15 +48,17 @@ public class MigrateAccountPatchReviewDb extends SiteProgram {
 
   @Override
   public int run() throws Exception {
+    Injector dbInjector = createDbInjector(DataSourceProvider.Context.SINGLE_USER);
     SitePaths sitePaths = new SitePaths(getSitePath());
+    ThreadSettingsConfig threadSettingsConfig = dbInjector.getInstance(ThreadSettingsConfig.class);
     Config fakeCfg = new Config();
     if (!Strings.isNullOrEmpty(sourceUrl)) {
       fakeCfg.setString("accountPatchReviewDb", null, "url", sourceUrl);
     }
     JdbcAccountPatchReviewStore sourceJdbcAccountPatchReviewStore =
-        JdbcAccountPatchReviewStore.createAccountPatchReviewStore(fakeCfg, sitePaths);
+        JdbcAccountPatchReviewStore.createAccountPatchReviewStore(
+            fakeCfg, sitePaths, threadSettingsConfig);
 
-    Injector dbInjector = createDbInjector(DataSourceProvider.Context.SINGLE_USER);
     Config cfg = dbInjector.getInstance(Key.get(Config.class, GerritServerConfig.class));
     String targetUrl = cfg.getString("accountPatchReviewDb", null, "url");
     if (targetUrl == null) {
@@ -64,7 +67,8 @@ public class MigrateAccountPatchReviewDb extends SiteProgram {
     }
     System.out.println("target Url: " + targetUrl);
     JdbcAccountPatchReviewStore targetJdbcAccountPatchReviewStore =
-        JdbcAccountPatchReviewStore.createAccountPatchReviewStore(cfg, sitePaths);
+        JdbcAccountPatchReviewStore.createAccountPatchReviewStore(
+            cfg, sitePaths, threadSettingsConfig);
     targetJdbcAccountPatchReviewStore.createTableIfNotExists();
 
     if (!isTargetTableEmpty(targetJdbcAccountPatchReviewStore)) {

gerrit-server/src/main/java/com/google/gerrit/server/schema/H2AccountPatchReviewStore.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.schema;
 
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.config.ThreadSettingsConfig;
 import com.google.gwtorm.server.OrmDuplicateKeyException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -27,8 +28,11 @@ import org.eclipse.jgit.lib.Config;
 public class H2AccountPatchReviewStore extends JdbcAccountPatchReviewStore {
 
   @Inject
-  H2AccountPatchReviewStore(@GerritServerConfig Config cfg, SitePaths sitePaths) {
-    super(cfg, sitePaths);
+  H2AccountPatchReviewStore(
+      @GerritServerConfig Config cfg,
+      SitePaths sitePaths,
+      ThreadSettingsConfig threadSettingsConfig) {
+    super(cfg, sitePaths, threadSettingsConfig);
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/schema/JdbcAccountPatchReviewStore.java

@@ -14,6 +14,9 @@
 
 package com.google.gerrit.server.schema;
 
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.SECONDS;
+
 import com.google.common.collect.ImmutableSet;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.extensions.events.LifecycleListener;
@@ -22,8 +25,10 @@ import com.google.gerrit.lifecycle.LifecycleModule;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.server.change.AccountPatchReviewStore;
+import com.google.gerrit.server.config.ConfigUtil;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.config.ThreadSettingsConfig;
 import com.google.gwtorm.server.OrmDuplicateKeyException;
 import com.google.gwtorm.server.OrmException;
 import java.sql.Connection;
@@ -77,29 +82,30 @@ public abstract class JdbcAccountPatchReviewStore
     }
   }
 
-  private final DataSource ds;
+  private DataSource ds;
 
   public static JdbcAccountPatchReviewStore createAccountPatchReviewStore(
-      Config cfg, SitePaths sitePaths) {
+      Config cfg, SitePaths sitePaths, ThreadSettingsConfig threadSettingsConfig) {
     String url = cfg.getString(ACCOUNT_PATCH_REVIEW_DB, null, URL);
     if (url == null || url.contains(H2_DB)) {
-      return new H2AccountPatchReviewStore(cfg, sitePaths);
+      return new H2AccountPatchReviewStore(cfg, sitePaths, threadSettingsConfig);
     }
     if (url.contains(POSTGRESQL)) {
-      return new PostgresqlAccountPatchReviewStore(cfg, sitePaths);
+      return new PostgresqlAccountPatchReviewStore(cfg, sitePaths, threadSettingsConfig);
     }
     if (url.contains(MYSQL)) {
-      return new MysqlAccountPatchReviewStore(cfg, sitePaths);
+      return new MysqlAccountPatchReviewStore(cfg, sitePaths, threadSettingsConfig);
     }
     if (url.contains(MARIADB)) {
-      return new MariaDBAccountPatchReviewStore(cfg, sitePaths);
+      return new MariaDBAccountPatchReviewStore(cfg, sitePaths, threadSettingsConfig);
     }
     throw new IllegalArgumentException(
         "unsupported driver type for account patch reviews db: " + url);
   }
 
-  protected JdbcAccountPatchReviewStore(Config cfg, SitePaths sitePaths) {
-    this.ds = createDataSource(getUrl(cfg, sitePaths));
+  protected JdbcAccountPatchReviewStore(
+      Config cfg, SitePaths sitePaths, ThreadSettingsConfig threadSettingsConfig) {
+    this.ds = createDataSource(cfg, sitePaths, threadSettingsConfig);
   }
 
   protected JdbcAccountPatchReviewStore(DataSource ds) {
@@ -114,27 +120,45 @@ public abstract class JdbcAccountPatchReviewStore
     return url;
   }
 
-  protected static DataSource createDataSource(String url) {
+  private static DataSource createDataSource(
+      Config cfg, SitePaths sitePaths, ThreadSettingsConfig threadSettingsConfig) {
     BasicDataSource datasource = new BasicDataSource();
-    if (url.contains(POSTGRESQL)) {
-      datasource.setDriverClassName("org.postgresql.Driver");
-    } else if (url.contains(H2_DB)) {
-      datasource.setDriverClassName("org.h2.Driver");
-    } else if (url.contains(MYSQL)) {
-      datasource.setDriverClassName("com.mysql.jdbc.Driver");
-    } else if (url.contains(MARIADB)) {
-      datasource.setDriverClassName("org.mariadb.jdbc.Driver");
-    }
+    String url = getUrl(cfg, sitePaths);
+    int poolLimit = threadSettingsConfig.getDatabasePoolLimit();
     datasource.setUrl(url);
-    datasource.setMaxActive(50);
-    datasource.setMinIdle(4);
-    datasource.setMaxIdle(16);
-    long evictIdleTimeMs = 1000 * 60;
+    datasource.setDriverClassName(getDriverFromUrl(url));
+    datasource.setMaxActive(cfg.getInt(ACCOUNT_PATCH_REVIEW_DB, "poolLimit", poolLimit));
+    datasource.setMinIdle(cfg.getInt(ACCOUNT_PATCH_REVIEW_DB, "poolminidle", 4));
+    datasource.setMaxIdle(
+        cfg.getInt(ACCOUNT_PATCH_REVIEW_DB, "poolmaxidle", Math.min(poolLimit, 16)));
+    datasource.setInitialSize(datasource.getMinIdle());
+    datasource.setMaxWait(
+        ConfigUtil.getTimeUnit(
+            cfg,
+            ACCOUNT_PATCH_REVIEW_DB,
+            null,
+            "poolmaxwait",
+            MILLISECONDS.convert(30, SECONDS),
+            MILLISECONDS));
+    long evictIdleTimeMs = 1000L * 60;
     datasource.setMinEvictableIdleTimeMillis(evictIdleTimeMs);
     datasource.setTimeBetweenEvictionRunsMillis(evictIdleTimeMs / 2);
     return datasource;
   }
 
+  private static String getDriverFromUrl(String url) {
+    if (url.contains(POSTGRESQL)) {
+      return "org.postgresql.Driver";
+    }
+    if (url.contains(MYSQL)) {
+      return "com.mysql.jdbc.Driver";
+    }
+    if (url.contains(MARIADB)) {
+      return "org.mariadb.jdbc.Driver";
+    }
+    return "org.h2.Driver";
+  }
+
   @Override
   public void start() {
     try {

gerrit-server/src/main/java/com/google/gerrit/server/schema/MariaDBAccountPatchReviewStore.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.schema;
 
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.config.ThreadSettingsConfig;
 import com.google.gwtorm.server.OrmDuplicateKeyException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -27,8 +28,11 @@ import org.eclipse.jgit.lib.Config;
 public class MariaDBAccountPatchReviewStore extends JdbcAccountPatchReviewStore {
 
   @Inject
-  MariaDBAccountPatchReviewStore(@GerritServerConfig Config cfg, SitePaths sitePaths) {
-    super(cfg, sitePaths);
+  MariaDBAccountPatchReviewStore(
+      @GerritServerConfig Config cfg,
+      SitePaths sitePaths,
+      ThreadSettingsConfig threadSettingsConfig) {
+    super(cfg, sitePaths, threadSettingsConfig);
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/schema/MysqlAccountPatchReviewStore.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.schema;
 
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.config.ThreadSettingsConfig;
 import com.google.gwtorm.server.OrmDuplicateKeyException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -27,8 +28,11 @@ import org.eclipse.jgit.lib.Config;
 public class MysqlAccountPatchReviewStore extends JdbcAccountPatchReviewStore {
 
   @Inject
-  MysqlAccountPatchReviewStore(@GerritServerConfig Config cfg, SitePaths sitePaths) {
-    super(cfg, sitePaths);
+  MysqlAccountPatchReviewStore(
+      @GerritServerConfig Config cfg,
+      SitePaths sitePaths,
+      ThreadSettingsConfig threadSettingsConfig) {
+    super(cfg, sitePaths, threadSettingsConfig);
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/schema/PostgresqlAccountPatchReviewStore.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.schema;
 
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.config.ThreadSettingsConfig;
 import com.google.gwtorm.server.OrmDuplicateKeyException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -27,8 +28,11 @@ import org.eclipse.jgit.lib.Config;
 public class PostgresqlAccountPatchReviewStore extends JdbcAccountPatchReviewStore {
 
   @Inject
-  PostgresqlAccountPatchReviewStore(@GerritServerConfig Config cfg, SitePaths sitePaths) {
-    super(cfg, sitePaths);
+  PostgresqlAccountPatchReviewStore(
+      @GerritServerConfig Config cfg,
+      SitePaths sitePaths,
+      ThreadSettingsConfig threadSettingsConfig) {
+    super(cfg, sitePaths, threadSettingsConfig);
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_127.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.schema;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.config.ThreadSettingsConfig;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -32,18 +33,25 @@ public class Schema_127 extends SchemaVersion {
 
   private final SitePaths sitePaths;
   private final Config cfg;
+  private final ThreadSettingsConfig threadSettingsConfig;
 
   @Inject
-  Schema_127(Provider<Schema_126> prior, SitePaths sitePaths, @GerritServerConfig Config cfg) {
+  Schema_127(
+      Provider<Schema_126> prior,
+      SitePaths sitePaths,
+      @GerritServerConfig Config cfg,
+      ThreadSettingsConfig threadSettingsConfig) {
     super(prior);
     this.sitePaths = sitePaths;
     this.cfg = cfg;
+    this.threadSettingsConfig = threadSettingsConfig;
   }
 
   @Override
   protected void migrateData(ReviewDb db, UpdateUI ui) throws OrmException {
     JdbcAccountPatchReviewStore jdbcAccountPatchReviewStore =
-        JdbcAccountPatchReviewStore.createAccountPatchReviewStore(cfg, sitePaths);
+        JdbcAccountPatchReviewStore.createAccountPatchReviewStore(
+            cfg, sitePaths, threadSettingsConfig);
     jdbcAccountPatchReviewStore.dropTableIfExists();
     jdbcAccountPatchReviewStore.createTableIfNotExists();
     try (Connection con = jdbcAccountPatchReviewStore.getConnection();