[GitHub] [gobblin] homatthew commented on a diff in pull request #3613: [GOBBLIN-1753] Migrate DB connection pool from o.a.commons.dbcp/dbcp2 to HikariCP

[GitBox <gi...@apache.org>]

homatthew commented on code in PR #3613:
URL: https://github.com/apache/gobblin/pull/3613#discussion_r1040140809


##########
gradle/scripts/dependencyDefinitions.gradle:
##########
@@ -70,6 +68,7 @@ ext.externalDependency = [
     "hadoopAws": "org.apache.hadoop:hadoop-aws:2.6.0",
     "hdrHistogram": "org.hdrhistogram:HdrHistogram:2.1.11",
     "helix": "org.apache.helix:helix-core:1.0.2",
+    "hikariCP": "com.zaxxer:HikariCP:3.2.0",

Review Comment:
   How did we choose this version over latest versions?



##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/db/ServiceDatabaseProviderImpl.java:
##########
@@ -61,25 +60,28 @@ private synchronized void ensureDataSource() {
       return;
     }
 
-    dataSource = new BasicDataSource();
+    dataSource = new HikariDataSource();
 
-    dataSource.setUrl(configuration.getUrl());
+    dataSource.setJdbcUrl(configuration.getUrl());
     dataSource.setUsername(configuration.getUserName());
     dataSource.setPassword(configuration.getPassword());
 
     // MySQL server can timeout a connection so need to validate connections before use
     final String validationQuery = MysqlDataSourceUtils.QUERY_CONNECTION_IS_VALID_AND_NOT_READONLY;
     LOG.info("setting `DataSource` validation query: '" + validationQuery + "'");
-    dataSource.setValidationQuery(validationQuery);
-    dataSource.setValidationQueryTimeout(5);
-
-    // To improve performance, we only check connections on creation, and set a maximum connection lifetime
+    // TODO: revisit following verification of successful connection pool migration:
+    //   If your driver supports JDBC4 we strongly recommend not setting this property. This is for "legacy" drivers
+    //   that do not support the JDBC4 Connection.isValid() API; see:
+    //   https://github.com/brettwooldridge/HikariCP#gear-configuration-knobs-baby
+    dataSource.setConnectionTestQuery(validationQuery);
+    dataSource.setValidationTimeout(5000);

Review Comment:
   Nit: Is there a reason we aren't using the `Duration.of` instead of a magic number



##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/MysqlUserQuotaManager.java:
##########
@@ -293,7 +293,8 @@ static class MysqlQuotaStore {
     private final String DECREASE_FLOWGROUP_COUNT_SQL;
     private final String DELETE_USER_SQL;
 
-    public MysqlQuotaStore(BasicDataSource dataSource, String tableName)
+    @edu.umd.cs.findbugs.annotations.SuppressFBWarnings("SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING")

Review Comment:
   I assume this warning was there before?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org