exceptionfactory commented on code in PR #10692:
URL: https://github.com/apache/nifi/pull/10692#discussion_r2649202734
##########
nifi-extension-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-hikari-dbcp-service/src/main/java/org/apache/nifi/dbcp/HikariCPConnectionPool.java:
##########
@@ -494,6 +503,79 @@ private void shutdown(final HikariDataSource dataSource,
final KerberosUser kerb
}
}
+ protected void loadDriver(final String driverName, final String url) {
+ final Class<?> clazz;
+
+ try {
+ clazz = Class.forName(driverName);
+ } catch (final ClassNotFoundException e) {
+ // Enhanced error message with discovery
+ ResourceReferences driverResources = null;
+ try {
+ // Try to get driver resources from current context if
available
+ if (getConfigurationContext() != null) {
+ driverResources =
getConfigurationContext().getProperty(DB_DRIVER_LOCATION).evaluateAttributeExpressions().asResources();
+ }
+ } catch (Exception ignored) {
+ // Context might not be available, continue without it
+ }
+
+ final List<String> availableDrivers = (driverResources != null &&
driverResources.getCount() != 0) ?
DriverUtils.discoverDriverClasses(driverResources) : List.of();
+
+ StringBuilder errorMessage = new StringBuilder("JDBC driver class
'%s' not found.".formatted(driverName));
+
+ if (!availableDrivers.isEmpty()) {
+ errorMessage.append(" Available driver classes found in
resources: %s.".formatted(String.join(", ", availableDrivers)));
+ } else if (driverResources != null && driverResources.getCount()
!= 0) {
+ final List<ResourceReference> resourcesList =
driverResources.asList();
+ if (resourcesList.stream().filter(r -> r.getResourceType() !=
ResourceType.URL).count() != 0) {
+ errorMessage.append(" No JDBC driver classes found in the
provided resources.");
+ }
+ } else if (driverResources == null) {
+ errorMessage.append(" The property 'Database Driver
Location(s)' should be set.");
+ }
+
+ throw new ProcessException(errorMessage.toString(), e);
+ }
+
+ try {
+ final Driver driver = DriverManager.getDriver(url);
+ // Ensure drivers that register themselves during class loading
can be set as the registeredDriver.
+ // This ensures drivers that register themselves can be
deregisterd when the componet is removed.
+ // These drivers should be loaded in the same InstanceClassloader
that load this component
+ if (driver != registeredDriver
+ &&
driver.getClass().getClassLoader().equals(getClass().getClassLoader())) {
+ DriverManager.deregisterDriver(registeredDriver);
+ registeredDriver = driver;
+ }
+ } catch (final SQLException e) {
+ // In case the driver is not registered by the implementation, we
explicitly try to register it.
+ try {
+ if (registeredDriver != null) {
+ DriverManager.deregisterDriver(registeredDriver);
+ }
+ registeredDriver = (Driver)
clazz.getDeclaredConstructor().newInstance();
+
+ DriverManager.registerDriver(registeredDriver);
+ DriverManager.getDriver(url);
+ } catch (final SQLException e2) {
+ throw new ProcessException("No suitable driver for the given
Database Connection URL", e2);
+ } catch (final Exception e2) {
+ throw new ProcessException("Creating driver instance is
failed", e2);
+ }
+ }
+ }
+
+ @OnRemoved
+ public void onRemove() {
+ try {
+ // We need to deregister the driver to allow the
InstanceClassLoader to be garbage collected.
+ DriverManager.deregisterDriver(registeredDriver);
+ } catch (SQLException e) {
+ getLogger().warn("Driver could not be deregistered. This may cause
a memory leak.", e);
Review Comment:
It would be helpful to include the driver class in the message, and it is
best to avoid ending log messages with the `.` character.
```suggestion
getLogger().warn("Failed to deregister driver [{}]",
registeredDriver, e);
```
##########
nifi-extension-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-hikari-dbcp-service/src/main/java/org/apache/nifi/dbcp/HikariCPConnectionPool.java:
##########
@@ -213,6 +217,8 @@ public class HikariCPConnectionPool extends
AbstractControllerService implements
private volatile HikariDataSource dataSource;
private volatile KerberosUser kerberosUser;
+ // Hold an instance of the driver so we can properly de-register it.
+ private volatile Driver registeredDriver;
Review Comment:
Is it necessary to keep this instance, or could `DriverManager.getDrivers()`
be used to get registered drivers and compare against the configured class name?
##########
nifi-extension-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-hikari-dbcp-service/src/main/java/org/apache/nifi/dbcp/HikariCPConnectionPool.java:
##########
@@ -494,6 +503,79 @@ private void shutdown(final HikariDataSource dataSource,
final KerberosUser kerb
}
}
+ protected void loadDriver(final String driverName, final String url) {
+ final Class<?> clazz;
+
+ try {
+ clazz = Class.forName(driverName);
+ } catch (final ClassNotFoundException e) {
+ // Enhanced error message with discovery
+ ResourceReferences driverResources = null;
+ try {
+ // Try to get driver resources from current context if
available
+ if (getConfigurationContext() != null) {
+ driverResources =
getConfigurationContext().getProperty(DB_DRIVER_LOCATION).evaluateAttributeExpressions().asResources();
+ }
+ } catch (Exception ignored) {
+ // Context might not be available, continue without it
+ }
+
+ final List<String> availableDrivers = (driverResources != null &&
driverResources.getCount() != 0) ?
DriverUtils.discoverDriverClasses(driverResources) : List.of();
+
+ StringBuilder errorMessage = new StringBuilder("JDBC driver class
'%s' not found.".formatted(driverName));
+
+ if (!availableDrivers.isEmpty()) {
+ errorMessage.append(" Available driver classes found in
resources: %s.".formatted(String.join(", ", availableDrivers)));
+ } else if (driverResources != null && driverResources.getCount()
!= 0) {
+ final List<ResourceReference> resourcesList =
driverResources.asList();
+ if (resourcesList.stream().filter(r -> r.getResourceType() !=
ResourceType.URL).count() != 0) {
+ errorMessage.append(" No JDBC driver classes found in the
provided resources.");
+ }
+ } else if (driverResources == null) {
+ errorMessage.append(" The property 'Database Driver
Location(s)' should be set.");
+ }
+
+ throw new ProcessException(errorMessage.toString(), e);
+ }
+
+ try {
+ final Driver driver = DriverManager.getDriver(url);
+ // Ensure drivers that register themselves during class loading
can be set as the registeredDriver.
+ // This ensures drivers that register themselves can be
deregisterd when the componet is removed.
+ // These drivers should be loaded in the same InstanceClassloader
that load this component
+ if (driver != registeredDriver
+ &&
driver.getClass().getClassLoader().equals(getClass().getClassLoader())) {
+ DriverManager.deregisterDriver(registeredDriver);
+ registeredDriver = driver;
+ }
+ } catch (final SQLException e) {
+ // In case the driver is not registered by the implementation, we
explicitly try to register it.
+ try {
+ if (registeredDriver != null) {
+ DriverManager.deregisterDriver(registeredDriver);
+ }
+ registeredDriver = (Driver)
clazz.getDeclaredConstructor().newInstance();
+
+ DriverManager.registerDriver(registeredDriver);
+ DriverManager.getDriver(url);
+ } catch (final SQLException e2) {
+ throw new ProcessException("No suitable driver for the given
Database Connection URL", e2);
+ } catch (final Exception e2) {
+ throw new ProcessException("Creating driver instance is
failed", e2);
Review Comment:
`ProcessException` may not be the best option in these cases. Perhaps
`IllegalStateException` would be better, but open to additional consideration.
##########
nifi-extension-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-hikari-dbcp-service/pom.xml:
##########
@@ -53,5 +53,29 @@
<artifactId>nifi-dbcp-utils</artifactId>
<version>2.8.0-SNAPSHOT</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.derby</groupId>
+ <artifactId>derby</artifactId>
Review Comment:
With the Apache Derby project now retired, it would be best to avoid
introducing additional dependencies on the library. I am evaluating other
options.
##########
nifi-extension-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-hikari-dbcp-service/src/main/java/org/apache/nifi/dbcp/HikariCPConnectionPool.java:
##########
@@ -494,6 +503,79 @@ private void shutdown(final HikariDataSource dataSource,
final KerberosUser kerb
}
}
+ protected void loadDriver(final String driverName, final String url) {
Review Comment:
```suggestion
protected void loadDriver(final String driverClassName, final String
url) {
```
##########
nifi-extension-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-hikari-dbcp-service/src/main/java/org/apache/nifi/dbcp/HikariCPConnectionPool.java:
##########
@@ -494,6 +503,79 @@ private void shutdown(final HikariDataSource dataSource,
final KerberosUser kerb
}
}
+ protected void loadDriver(final String driverName, final String url) {
+ final Class<?> clazz;
+
+ try {
+ clazz = Class.forName(driverName);
+ } catch (final ClassNotFoundException e) {
+ // Enhanced error message with discovery
+ ResourceReferences driverResources = null;
+ try {
+ // Try to get driver resources from current context if
available
+ if (getConfigurationContext() != null) {
+ driverResources =
getConfigurationContext().getProperty(DB_DRIVER_LOCATION).evaluateAttributeExpressions().asResources();
+ }
+ } catch (Exception ignored) {
+ // Context might not be available, continue without it
+ }
+
+ final List<String> availableDrivers = (driverResources != null &&
driverResources.getCount() != 0) ?
DriverUtils.discoverDriverClasses(driverResources) : List.of();
+
+ StringBuilder errorMessage = new StringBuilder("JDBC driver class
'%s' not found.".formatted(driverName));
+
+ if (!availableDrivers.isEmpty()) {
+ errorMessage.append(" Available driver classes found in
resources: %s.".formatted(String.join(", ", availableDrivers)));
+ } else if (driverResources != null && driverResources.getCount()
!= 0) {
+ final List<ResourceReference> resourcesList =
driverResources.asList();
+ if (resourcesList.stream().filter(r -> r.getResourceType() !=
ResourceType.URL).count() != 0) {
+ errorMessage.append(" No JDBC driver classes found in the
provided resources.");
+ }
+ } else if (driverResources == null) {
+ errorMessage.append(" The property 'Database Driver
Location(s)' should be set.");
+ }
+
+ throw new ProcessException(errorMessage.toString(), e);
+ }
+
+ try {
+ final Driver driver = DriverManager.getDriver(url);
+ // Ensure drivers that register themselves during class loading
can be set as the registeredDriver.
+ // This ensures drivers that register themselves can be
deregisterd when the componet is removed.
+ // These drivers should be loaded in the same InstanceClassloader
that load this component
+ if (driver != registeredDriver
+ &&
driver.getClass().getClassLoader().equals(getClass().getClassLoader())) {
+ DriverManager.deregisterDriver(registeredDriver);
+ registeredDriver = driver;
+ }
+ } catch (final SQLException e) {
+ // In case the driver is not registered by the implementation, we
explicitly try to register it.
+ try {
+ if (registeredDriver != null) {
+ DriverManager.deregisterDriver(registeredDriver);
+ }
+ registeredDriver = (Driver)
clazz.getDeclaredConstructor().newInstance();
+
+ DriverManager.registerDriver(registeredDriver);
+ DriverManager.getDriver(url);
+ } catch (final SQLException e2) {
+ throw new ProcessException("No suitable driver for the given
Database Connection URL", e2);
+ } catch (final Exception e2) {
+ throw new ProcessException("Creating driver instance is
failed", e2);
+ }
+ }
+ }
+
+ @OnRemoved
+ public void onRemove() {
+ try {
+ // We need to deregister the driver to allow the
InstanceClassLoader to be garbage collected.
+ DriverManager.deregisterDriver(registeredDriver);
+ } catch (SQLException e) {
Review Comment:
```suggestion
} catch (final SQLException e) {
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
