Remove minimumIdle config in HikariCP (#557)

* Remove minimumIdle config for HikariCP

* Add comment

* Resolve comment

core/src/main/java/google/registry/config/RegistryConfig.java

@@ -1512,21 +1512,11 @@ public final class RegistryConfig {
     return CONFIG_SETTINGS.get().hibernate.hikariConnectionTimeout;
   }
 
-  /** Returns the minimum idle connections for HikariCP. */
-  public static String getHibernateHikariMinimumIdle() {
-    return CONFIG_SETTINGS.get().hibernate.hikariMinimumIdle;
-  }
-
   /** Returns the maximum pool size for HikariCP. */
   public static String getHibernateHikariMaximumPoolSize() {
     return CONFIG_SETTINGS.get().hibernate.hikariMaximumPoolSize;
   }
 
-  /** Returns the idle timeout for HikariCP. */
-  public static String getHibernateHikariIdleTimeout() {
-    return CONFIG_SETTINGS.get().hibernate.hikariIdleTimeout;
-  }
-
   /** Returns the roid suffix to be used for the roids of all contacts and hosts. */
   public static String getContactAndHostRoidSuffix() {
     return CONFIG_SETTINGS.get().registryPolicy.contactAndHostRoidSuffix;

core/src/main/java/google/registry/config/RegistryConfigSettings.java

@@ -112,9 +112,7 @@ public class RegistryConfigSettings {
     public String connectionIsolation;
     public String logSqlQueries;
     public String hikariConnectionTimeout;
-    public String hikariMinimumIdle;
     public String hikariMaximumPoolSize;
-    public String hikariIdleTimeout;
   }
 
   /** Configuration for Cloud SQL. */

core/src/main/java/google/registry/config/files/default-config.yaml

@@ -207,9 +207,17 @@ hibernate:
 
   # Connection pool configurations.
   hikariConnectionTimeout: 20000
-  hikariMinimumIdle: 0
+
+  # We occasionally received "Connection is not available, request timed out"
+  # exception when setting minimumIdle to 0 and it turned out it is a bug (See
+  # https://github.com/brettwooldridge/HikariCP/issues/1212) in HikariCP and
+  # the workaround is to use a fixed size connection pool.
+  #
+  # HikariCP also recommends not setting minimumIdle for maximum performance
+  # and responsiveness to spike demands (See
+  # https://github.com/brettwooldridge/HikariCP).
+  # TODO(b/154720215): Experiment with a smaller pool size.
   hikariMaximumPoolSize: 20
-  hikariIdleTimeout: 300000
 
 cloudSql:
   # jdbc url for the Cloud SQL database.

core/src/main/java/google/registry/persistence/PersistenceModule.java

@@ -17,9 +17,7 @@ package google.registry.persistence;
 import static com.google.common.base.Preconditions.checkState;
 import static google.registry.config.RegistryConfig.getHibernateConnectionIsolation;
 import static google.registry.config.RegistryConfig.getHibernateHikariConnectionTimeout;
-import static google.registry.config.RegistryConfig.getHibernateHikariIdleTimeout;
 import static google.registry.config.RegistryConfig.getHibernateHikariMaximumPoolSize;
-import static google.registry.config.RegistryConfig.getHibernateHikariMinimumIdle;
 import static google.registry.config.RegistryConfig.getHibernateLogSqlQueries;
 
 import com.google.api.client.auth.oauth2.Credential;
@@ -50,10 +48,7 @@ public class PersistenceModule {
   // This name must be the same as the one defined in persistence.xml.
   public static final String PERSISTENCE_UNIT_NAME = "nomulus";
   public static final String HIKARI_CONNECTION_TIMEOUT = "hibernate.hikari.connectionTimeout";
-  public static final String HIKARI_MINIMUM_IDLE = "hibernate.hikari.minimumIdle";
   public static final String HIKARI_MAXIMUM_POOL_SIZE = "hibernate.hikari.maximumPoolSize";
-  public static final String HIKARI_IDLE_TIMEOUT = "hibernate.hikari.idleTimeout";
-
   public static final String HIKARI_DS_SOCKET_FACTORY = "hibernate.hikari.dataSource.socketFactory";
   public static final String HIKARI_DS_CLOUD_SQL_INSTANCE =
       "hibernate.hikari.dataSource.cloudSqlInstance";
@@ -79,9 +74,7 @@ public class PersistenceModule {
     properties.put(Environment.ISOLATION, getHibernateConnectionIsolation());
     properties.put(Environment.SHOW_SQL, getHibernateLogSqlQueries());
     properties.put(HIKARI_CONNECTION_TIMEOUT, getHibernateHikariConnectionTimeout());
-    properties.put(HIKARI_MINIMUM_IDLE, getHibernateHikariMinimumIdle());
     properties.put(HIKARI_MAXIMUM_POOL_SIZE, getHibernateHikariMaximumPoolSize());
-    properties.put(HIKARI_IDLE_TIMEOUT, getHibernateHikariIdleTimeout());
     properties.put(Environment.DIALECT, NomulusPostgreSQLDialect.class.getName());
     return properties.build();
   }

core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerRule.java

@@ -41,6 +41,7 @@ import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.sql.Statement;
 import java.util.HashMap;
+import java.util.Map;
 import java.util.Optional;
 import java.util.Properties;
 import java.util.stream.Collectors;
@@ -76,7 +77,7 @@ abstract class JpaTransactionManagerRule extends ExternalResource {
   private final Clock clock;
   private final Optional<String> initScriptPath;
   private final ImmutableList<Class> extraEntityClasses;
-  private final ImmutableMap userProperties;
+  private final ImmutableMap<String, String> userProperties;
 
   private static final JdbcDatabaseContainer database = create();
   private static final HibernateSchemaExporter exporter =
@@ -143,15 +144,18 @@ abstract class JpaTransactionManagerRule extends ExternalResource {
           new String(Files.readAllBytes(tempSqlFile.toPath()), StandardCharsets.UTF_8));
     }
 
-    ImmutableMap properties = PersistenceModule.providesDefaultDatabaseConfigs();
+    Map<String, String> properties =
+        Maps.newHashMap(PersistenceModule.providesDefaultDatabaseConfigs());
+    // Set the minimumIdle to 0 so assertReasonableNumDbConnections() can check if there is
+    // connection leak after each test.
+    properties.put("hibernate.hikari.minimumIdle", "0");
+    properties.put("hibernate.hikari.idleTimeout", "300000");
     if (!userProperties.isEmpty()) {
       // If there are user properties, create a new properties object with these added.
-      ImmutableMap.Builder builder = properties.builder();
-      builder.putAll(userProperties);
+      properties.putAll(userProperties);
       // Forbid Hibernate push to stay consistent with flyway-based schema management.
-      builder.put(Environment.HBM2DDL_AUTO, "none");
-      builder.put(Environment.SHOW_SQL, "true");
-      properties = builder.build();
+      properties.put(Environment.HBM2DDL_AUTO, "none");
+      properties.put(Environment.SHOW_SQL, "true");
     }
     assertReasonableNumDbConnections();
     emf =
@@ -159,7 +163,7 @@ abstract class JpaTransactionManagerRule extends ExternalResource {
             getJdbcUrl(),
             database.getUsername(),
             database.getPassword(),
-            properties,
+            ImmutableMap.copyOf(properties),
             extraEntityClasses);
     emfEntityHash = entityHash;
   }