Merge pull request #428 from lutovich/1.5-default-config-values

Improve default config values for connection pool

Author: lutovich

driver/src/main/java/org/neo4j/driver/internal/async/pool/PoolSettings.java

@@ -18,17 +18,17 @@
  */
 package org.neo4j.driver.internal.async.pool;
 
+import java.util.concurrent.TimeUnit;
+
 public class PoolSettings
 {
-    public static final int NO_IDLE_CONNECTION_TEST = -1;
-    public static final int INFINITE_CONNECTION_LIFETIME = -1;
     public static final int NOT_CONFIGURED = -1;
 
-    public static final int DEFAULT_MAX_IDLE_CONNECTION_POOL_SIZE = 10;
-    public static final int DEFAULT_MAX_CONNECTION_POOL_SIZE = Integer.MAX_VALUE;
+    public static final int DEFAULT_MAX_CONNECTION_POOL_SIZE = 100;
+    public static final int DEFAULT_MAX_IDLE_CONNECTION_POOL_SIZE = DEFAULT_MAX_CONNECTION_POOL_SIZE;
     public static final long DEFAULT_IDLE_TIME_BEFORE_CONNECTION_TEST = NOT_CONFIGURED;
-    public static final long DEFAULT_MAX_CONNECTION_LIFETIME = NOT_CONFIGURED;
-    public static final long DEFAULT_CONNECTION_ACQUISITION_TIMEOUT = Long.MAX_VALUE;
+    public static final long DEFAULT_MAX_CONNECTION_LIFETIME = TimeUnit.HOURS.toMillis( 1 );
+    public static final long DEFAULT_CONNECTION_ACQUISITION_TIMEOUT = TimeUnit.SECONDS.toMillis( 60 );
 
     private final int maxIdleConnectionPoolSize;
     private final long idleTimeBeforeConnectionTest;

driver/src/main/java/org/neo4j/driver/v1/Config.java

@@ -22,9 +22,9 @@
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
 
+import org.neo4j.driver.internal.async.pool.PoolSettings;
 import org.neo4j.driver.internal.cluster.RoutingSettings;
 import org.neo4j.driver.internal.logging.JULogging;
-import org.neo4j.driver.internal.async.pool.PoolSettings;
 import org.neo4j.driver.internal.retry.RetrySettings;
 import org.neo4j.driver.v1.exceptions.ServiceUnavailableException;
 import org.neo4j.driver.v1.exceptions.SessionExpiredException;
@@ -412,7 +412,7 @@ public ConfigBuilder withConnectionLivenessCheckTimeout( long value, TimeUnit un
          * this case, it is recommended to set liveness check to a value smaller than network equipment has and maximum
          * lifetime to a reasonably large value to "renew" connections once in a while.
          * <p>
-         * No maximum lifetime limit is imposed by default. Zero and negative values result in lifetime not being
+         * Default maximum connection lifetime is 1 hour. Zero and negative values result in lifetime not being
          * checked.
          *
          * @param value the maximum connection lifetime
@@ -426,27 +426,64 @@ public ConfigBuilder withMaxConnectionLifetime( long value, TimeUnit unit )
         }
 
         /**
-         * Todo: doc and validation
+         * Configure maximum amount of connections in the connection pool towards a single database. This setting
+         * limits total amount of connections in the pool when used in direct driver, created for URI with 'bolt'
+         * scheme. It will limit amount of connections per cluster member when used with routing driver, created for
+         * URI with 'bolt+routing' scheme.
+         * <p>
+         * Acquisition will be attempted for at most configured timeout
+         * {@link #withConnectionAcquisitionTimeout(long, TimeUnit)} when limit is reached.
+         * <p>
+         * Default value is {@code 100}. Negative values are allowed and result in unlimited pool. Value of {@code 0}
+         * is not allowed.
          *
-         * @param value
-         * @return
+         * @param value the maximum connection pool size.
+         * @return this builder
+         * @see #withConnectionAcquisitionTimeout(long, TimeUnit)
          */
         public ConfigBuilder withMaxConnectionPoolSize( int value )
         {
-            this.maxConnectionPoolSize = value;
+            if ( value == 0 )
+            {
+                throw new IllegalArgumentException( "Zero value is not supported" );
+            }
+            else if ( value < 0 )
+            {
+                this.maxConnectionPoolSize = Integer.MAX_VALUE;
+            }
+            else
+            {
+                this.maxConnectionPoolSize = value;
+            }
             return this;
         }
 
         /**
-         * Todo: doc and validation
+         * Configure maximum amount of time connection acquisition will attempt to acquire a connection from the
+         * connection pool. This timeout only kicks in when all existing connections are being used and no new
+         * connections can be created because maximum connection pool size has been reached.
+         * <p>
+         * Exception is raised when connection can't be acquired within configured time.
+         * <p>
+         * Default value is 60 seconds. Negative values are allowed and result in unlimited acquisition timeout. Value
+         * of {@code 0} is allowed and results in no timeout and immediate failure when connection is unavailable.
          *
-         * @param value
-         * @param unit
-         * @return
+         * @param value the acquisition timeout
+         * @param unit the unit in which the duration is given
+         * @return this builder
+         * @see #withMaxConnectionPoolSize(int)
          */
         public ConfigBuilder withConnectionAcquisitionTimeout( long value, TimeUnit unit )
         {
-            this.connectionAcquisitionTimeoutMillis = unit.toMillis( value );
+            long valueInMillis = unit.toMillis( value );
+            if ( value >= 0 )
+            {
+                this.connectionAcquisitionTimeoutMillis = valueInMillis;
+            }
+            else
+            {
+                this.connectionAcquisitionTimeoutMillis = -1;
+            }
             return this;
         }
 

driver/src/test/java/org/neo4j/driver/v1/ConfigTest.java

@@ -106,6 +106,12 @@ public void shouldAllowNegativeConnectionLivenessCheckTimeout() throws Throwable
         assertEquals( TimeUnit.SECONDS.toMillis( -42 ), config.idleTimeBeforeConnectionTest() );
     }
 
+    @Test
+    public void shouldHaveCorrectMaxConnectionLifetime()
+    {
+        assertEquals( TimeUnit.HOURS.toMillis( 1 ), Config.defaultConfig().maxConnectionLifetimeMillis() );
+    }
+
     @Test
     public void shouldSupportMaxConnectionLifetimeSetting() throws Throwable
     {
@@ -225,6 +231,72 @@ public void shouldAllowPositiveRetryAttempts()
         assertEquals( TimeUnit.SECONDS.toMillis( 42 ), config.retrySettings().maxRetryTimeMs() );
     }
 
+    @Test
+    public void shouldHaveCorrectDefaultMaxConnectionPoolSize()
+    {
+        assertEquals( 100, Config.defaultConfig().maxConnectionPoolSize() );
+    }
+
+    @Test
+    public void shouldAllowPositiveMaxConnectionPoolSize()
+    {
+        Config config = Config.build().withMaxConnectionPoolSize( 42 ).toConfig();
+
+        assertEquals( 42, config.maxConnectionPoolSize() );
+    }
+
+    @Test
+    public void shouldAllowNegativeMaxConnectionPoolSize()
+    {
+        Config config = Config.build().withMaxConnectionPoolSize( -42 ).toConfig();
+
+        assertEquals( Integer.MAX_VALUE, config.maxConnectionPoolSize() );
+    }
+
+    @Test
+    public void shouldDisallowZeroMaxConnectionPoolSize()
+    {
+        try
+        {
+            Config.build().withMaxConnectionPoolSize( 0 ).toConfig();
+            fail( "Exception expected" );
+        }
+        catch ( IllegalArgumentException e )
+        {
+            assertEquals( "Zero value is not supported", e.getMessage() );
+        }
+    }
+
+    @Test
+    public void shouldHaveCorrectDefaultConnectionAcquisitionTimeout()
+    {
+        assertEquals( TimeUnit.SECONDS.toMillis( 60 ), Config.defaultConfig().connectionAcquisitionTimeoutMillis() );
+    }
+
+    @Test
+    public void shouldAllowPositiveConnectionAcquisitionTimeout()
+    {
+        Config config = Config.build().withConnectionAcquisitionTimeout( 42, TimeUnit.SECONDS ).toConfig();
+
+        assertEquals( TimeUnit.SECONDS.toMillis( 42 ), config.connectionAcquisitionTimeoutMillis() );
+    }
+
+    @Test
+    public void shouldAllowNegativeConnectionAcquisitionTimeout()
+    {
+        Config config = Config.build().withConnectionAcquisitionTimeout( -42, TimeUnit.HOURS ).toConfig();
+
+        assertEquals( -1, config.connectionAcquisitionTimeoutMillis() );
+    }
+
+    @Test
+    public void shouldAllowConnectionAcquisitionTimeoutOfZero()
+    {
+        Config config = Config.build().withConnectionAcquisitionTimeout( 0, TimeUnit.DAYS ).toConfig();
+
+        assertEquals( 0, config.connectionAcquisitionTimeoutMillis() );
+    }
+
     public static void deleteDefaultKnownCertFileIfExists()
     {
         if( DEFAULT_KNOWN_HOSTS.exists() )