Add ability to specify dbcp properties file (#6419)

* Add ability to specify dbcp properties file

* Address PR comments, use mock config, remove setter

* Add documentation

* APRC, updated docs with example file contents

* APRC, add @Nullable, @VisibileForTesting, update doc

* APRC, remove error log, use props directly as jackson binding

* Remove unused files

core/src/main/java/org/apache/druid/metadata/MetadataStorageConnectorConfig.java

@@ -21,6 +21,7 @@ package org.apache.druid.metadata;
 
 import com.fasterxml.jackson.annotation.JsonProperty;
 import org.apache.druid.java.util.common.StringUtils;
+import java.util.Properties;
 
 /**
  */
@@ -44,6 +45,9 @@ public class MetadataStorageConnectorConfig
   @JsonProperty("password")
   private PasswordProvider passwordProvider;
 
+  @JsonProperty("dbcp")
+  private Properties dbcpProperties;
+
   public boolean isCreateTables()
   {
     return createTables;
@@ -78,6 +82,11 @@ public class MetadataStorageConnectorConfig
     return passwordProvider == null ? null : passwordProvider.getPassword();
   }
 
+  public Properties getDbcpProperties()
+  {
+    return dbcpProperties;
+  }
+
   @Override
   public String toString()
   {
@@ -86,6 +95,7 @@ public class MetadataStorageConnectorConfig
            ", connectURI='" + getConnectURI() + '\'' +
            ", user='" + user + '\'' +
            ", passwordProvider=" + passwordProvider +
+           ", dbcpProperties=" + dbcpProperties +
            '}';
   }
 
@@ -116,6 +126,11 @@ public class MetadataStorageConnectorConfig
     if (getUser() != null ? !getUser().equals(that.getUser()) : that.getUser() != null) {
       return false;
     }
+    if (getDbcpProperties() == null
+        ? that.getDbcpProperties() != null
+        : !getDbcpProperties().equals(that.getDbcpProperties())) {
+      return false;
+    }
     return passwordProvider != null ? passwordProvider.equals(that.passwordProvider) : that.passwordProvider == null;
 
   }
@@ -129,6 +144,7 @@ public class MetadataStorageConnectorConfig
     result = 31 * result + (getConnectURI() != null ? getConnectURI().hashCode() : 0);
     result = 31 * result + (getUser() != null ? getUser().hashCode() : 0);
     result = 31 * result + (passwordProvider != null ? passwordProvider.hashCode() : 0);
+    result = 31 * result + (dbcpProperties != null ? dbcpProperties.hashCode() : 0);
     return result;
   }
 }

core/src/test/java/org/apache/druid/metadata/MetadataStorageConnectorConfigTest.java

@@ -24,6 +24,7 @@ import org.junit.Assert;
 import org.junit.Test;
 
 import java.io.IOException;
+import java.util.Properties;
 
 public class MetadataStorageConnectorConfigTest
 {
@@ -45,7 +46,11 @@ public class MetadataStorageConnectorConfigTest
         "\"port\": \"" + port + "\"," +
         "\"connectURI\": \"" + connectURI + "\"," +
         "\"user\": \"" + user + "\"," +
-        "\"password\": " + pwdString +
+        "\"password\": " + pwdString + "," +
+        "\"dbcp\": {\n" +
+        "  \"maxConnLifetimeMillis\" : 1200000,\n" +
+        "  \"defaultQueryTimeout\" : \"30000\"\n" +
+        "}" +
         "}",
         MetadataStorageConnectorConfig.class
     );
@@ -71,6 +76,7 @@ public class MetadataStorageConnectorConfigTest
         "\"nothing\""
     );
     Assert.assertTrue(metadataStorageConnectorConfig.equals(metadataStorageConnectorConfig2));
+    Assert.assertTrue(metadataStorageConnectorConfig.hashCode() == metadataStorageConnectorConfig2.hashCode());
   }
 
   private static final ObjectMapper jsonMapper = new ObjectMapper();
@@ -130,5 +136,55 @@ public class MetadataStorageConnectorConfigTest
     Assert.assertEquals(connectURI, config.getConnectURI());
     Assert.assertEquals(user, config.getUser());
     Assert.assertEquals(pwd, config.getPassword());
+    Assert.assertNull(config.getDbcpProperties());
+  }
+
+  @Test
+  public void testDbcpProperties() throws Exception
+  {
+    testDbcpPropertiesFile(
+        true,
+        "host",
+        1234,
+        "connectURI",
+        "user",
+        "{\"type\":\"default\",\"password\":\"nothing\"}",
+        "nothing"
+    );
+  }
+  private void testDbcpPropertiesFile(
+          boolean createTables,
+          String host,
+          int port,
+          String connectURI,
+          String user,
+          String pwdString,
+          String pwd
+  ) throws Exception
+  {
+    MetadataStorageConnectorConfig config = jsonMapper.readValue(
+            "{" +
+                    "\"createTables\": \"" + createTables + "\"," +
+                    "\"host\": \"" + host + "\"," +
+                    "\"port\": \"" + port + "\"," +
+                    "\"connectURI\": \"" + connectURI + "\"," +
+                    "\"user\": \"" + user + "\"," +
+                    "\"password\": " + pwdString + "," +
+                    "\"dbcp\": {\n" +
+                    "  \"maxConnLifetimeMillis\" : 1200000,\n" +
+                    "  \"defaultQueryTimeout\" : \"30000\"\n" +
+                    "}" +
+                    "}",
+            MetadataStorageConnectorConfig.class
+    );
+
+    Assert.assertEquals(host, config.getHost());
+    Assert.assertEquals(port, config.getPort());
+    Assert.assertEquals(connectURI, config.getConnectURI());
+    Assert.assertEquals(user, config.getUser());
+    Assert.assertEquals(pwd, config.getPassword());
+    Properties dbcpProperties = config.getDbcpProperties();
+    Assert.assertEquals(dbcpProperties.getProperty("maxConnLifetimeMillis"), "1200000");
+    Assert.assertEquals(dbcpProperties.getProperty("defaultQueryTimeout"), "30000");
   }
 }

docs/content/dependencies/metadata-storage.md

@@ -32,6 +32,19 @@ See [mysql-metadata-storage extension documentation](../development/extensions-c
 
 See [postgresql-metadata-storage](../development/extensions-core/postgresql.html). 
 
+## Adding custom dbcp properties
+
+NOTE: These properties are not settable through the druid.metadata.storage.connector.dbcp properties : username, password, connectURI, validationQuery, testOnBorrow. These must be set through druid.metadata.storage.connector properties.
+
+Example supported properties:
+
+```properties
+druid.metadata.storage.connector.dbcp.maxConnLifetimeMillis=1200000
+druid.metadata.storage.connector.dbcp.defaultQueryTimeout=30000
+```
+
+See [BasicDataSource Configuration](https://commons.apache.org/proper/commons-dbcp/configuration.html) for full list.
+
 ## Metadata Storage Tables
 
 ### Segments Table

server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java

@@ -24,6 +24,7 @@ import com.google.common.base.Supplier;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import org.apache.commons.dbcp2.BasicDataSource;
+import org.apache.commons.dbcp2.BasicDataSourceFactory;
 import org.apache.druid.java.util.common.ISE;
 import org.apache.druid.java.util.common.RetryUtils;
 import org.apache.druid.java.util.common.StringUtils;
@@ -47,6 +48,7 @@ import java.sql.SQLTransientException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Properties;
 
 public abstract class SQLMetadataConnector implements MetadataStorageConnector
 {
@@ -636,7 +638,20 @@ public abstract class SQLMetadataConnector implements MetadataStorageConnector
   {
     MetadataStorageConnectorConfig connectorConfig = getConfig();
 
-    BasicDataSource dataSource = new BasicDataSource();
+    BasicDataSource dataSource = null;
+
+    try {
+      Properties dbcpProperties = connectorConfig.getDbcpProperties();
+      if (dbcpProperties != null) {
+        dataSource = BasicDataSourceFactory.createDataSource(dbcpProperties);
+      } else {
+        dataSource = new BasicDataSource();
+      }
+    }
+    catch (Exception e) {
+      throw new RuntimeException(e);
+    }
+
     dataSource.setUsername(connectorConfig.getUser());
     dataSource.setPassword(connectorConfig.getPassword());
     String uri = connectorConfig.getConnectURI();

server/src/test/java/org/apache/druid/metadata/SQLMetadataConnectorTest.java

@@ -18,11 +18,16 @@
  */
 package org.apache.druid.metadata;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;
+import org.apache.commons.dbcp2.BasicDataSource;
 import org.apache.druid.java.util.common.StringUtils;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.skife.jdbi.v2.DBI;
 import org.skife.jdbi.v2.Handle;
 import org.skife.jdbi.v2.tweak.HandleCallback;
 
@@ -37,6 +42,7 @@ public class SQLMetadataConnectorTest
 
   private TestDerbyConnector connector;
   private MetadataStorageTablesConfig tablesConfig;
+  private static final ObjectMapper jsonMapper = new ObjectMapper();
 
   @Before
   public void setUp()
@@ -140,4 +146,99 @@ public class SQLMetadataConnectorTest
         }
     );
   }
+
+  static class TestSQLMetadataConnector extends SQLMetadataConnector
+  {
+    public TestSQLMetadataConnector(
+        Supplier<MetadataStorageConnectorConfig> config,
+        Supplier<MetadataStorageTablesConfig> tablesConfigSupplier
+    )
+    {
+      super(config, tablesConfigSupplier);
+    }
+
+    @Override
+    protected String getSerialType()
+    {
+      return null;
+    }
+
+    @Override
+    protected int getStreamingFetchSize()
+    {
+      return 0;
+    }
+
+    @Override
+    public String getQuoteString()
+    {
+      return null;
+    }
+
+    @Override
+    public boolean tableExists(Handle handle, String tableName)
+    {
+      return false;
+    }
+
+    @Override
+    public DBI getDBI()
+    {
+      return null;
+    }
+
+    @Override
+    protected BasicDataSource getDatasource()
+    {
+      return super.getDatasource();
+    }
+  }
+
+  private MetadataStorageConnectorConfig getDbcpPropertiesFile(
+      boolean createTables,
+      String host,
+      int port,
+      String connectURI,
+      String user,
+      String pwdString,
+      String pwd
+  ) throws Exception
+  {
+    return jsonMapper.readValue(
+        "{" +
+        "\"createTables\": \"" + createTables + "\"," +
+        "\"host\": \"" + host + "\"," +
+        "\"port\": \"" + port + "\"," +
+        "\"connectURI\": \"" + connectURI + "\"," +
+        "\"user\": \"" + user + "\"," +
+        "\"password\": " + pwdString + "," +
+        "\"dbcp\": {\n" +
+        "  \"maxConnLifetimeMillis\" : 1200000,\n" +
+        "  \"defaultQueryTimeout\" : \"30000\"\n" +
+        "}" +
+        "}",
+        MetadataStorageConnectorConfig.class
+    );
+  }
+
+  @Test
+  public void testBasicDataSourceCreation() throws Exception
+  {
+    MetadataStorageConnectorConfig config = getDbcpPropertiesFile(
+        true,
+        "host",
+        1234,
+        "connectURI",
+        "user",
+        "{\"type\":\"default\",\"password\":\"nothing\"}",
+        "nothing"
+    );
+    TestSQLMetadataConnector testSQLMetadataConnector = new TestSQLMetadataConnector(
+        Suppliers.ofInstance(config),
+        Suppliers.ofInstance(tablesConfig)
+    );
+    BasicDataSource dataSource = testSQLMetadataConnector.getDatasource();
+    Assert.assertEquals(dataSource.getMaxConnLifetimeMillis(), 1200000);
+    Assert.assertEquals((long) dataSource.getDefaultQueryTimeout(), 30000);
+  }
 }