[GitHub] [nifi] simonbence commented on a change in pull request #5003: NIFI-8429 Changing DBCPConnectionPool driver manager in order to prevent from leaking

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

simonbence commented on a change in pull request #5003:
URL: https://github.com/apache/nifi/pull/5003#discussion_r614092555



##########
File path: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java
##########
@@ -460,45 +455,21 @@ public void onConfigured(final ConfigurationContext context) throws Initializati
         });
     }
 
-    private Long extractMillisWithInfinite(PropertyValue prop) {
-        return "-1".equals(prop.getValue()) ? -1 : prop.asTimePeriod(TimeUnit.MILLISECONDS);
+    private Driver getDriver(final String driverName, final String url) {
+        try {
+            final Class<?> clazz = Class.forName(driverName);
+            final Driver driver = DriverManager.getDriver(url);
+
+            return (driver == null)

Review comment:
       Your concerns are valid, however it is hard to determine if the SQLException comes due to the driver implementation is not registering the driver instance as it is expected or because of the URL is incorrect. In both cases, the manager will not be able to return with a driver. But, as I said your concerns are valid, so I will go with the following implementation:
   
   1. Checking if DriverManager.getDriver() returns with a driver. If yes, we will use that.
   2. In case of SQLException, we try to remediate the situation with explicitly creating AND registering a driver instance
   3. We try with getDriver() again. If it returns with a driver we can use that. Otherwise, if even after explicitly registering a driver, there is no sufficient one to return, then it is safe to assume that the URL is not supported by the driver.
   
   I go with this, I hope this also covers your expectations




-- 
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.

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