Re: [PR] NIFI-12890: Refactor HadoopDBCPConnectionPool to extend AbstractDBCPConnectionPool [nifi]
asfgit closed pull request #8619: NIFI-12890: Refactor HadoopDBCPConnectionPool to extend AbstractDBCPConnectionPool URL: https://github.com/apache/nifi/pull/8619 -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12890: Refactor HadoopDBCPConnectionPool to extend AbstractDBCPConnectionPool [nifi]
turcsanyip commented on code in PR #8619:
URL: https://github.com/apache/nifi/pull/8619#discussion_r1559748008
##
nifi-nar-bundles/nifi-standard-services/nifi-hadoop-dbcp-service-bundle/nifi-hadoop-dbcp-service/src/main/java/org/apache/nifi/dbcp/HadoopDBCPConnectionPool.java:
##
@@ -64,15 +48,44 @@
import org.apache.nifi.security.krb.KerberosKeytabUser;
import org.apache.nifi.security.krb.KerberosLoginException;
import org.apache.nifi.security.krb.KerberosPasswordUser;
-import org.apache.nifi.security.krb.KerberosUser;
+
+import javax.security.auth.login.LoginException;
+import java.io.File;
+import java.io.IOException;
+import java.lang.reflect.UndeclaredThrowableException;
+import java.security.PrivilegedExceptionAction;
+import java.sql.Connection;
+import java.sql.Driver;
+import java.sql.DriverManager;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
+
+import static org.apache.nifi.dbcp.utils.DBCPProperties.DATABASE_URL;
Review Comment:
`DBCPProperties.KERBEROS_USER_SERVICE` should be used too instead of
defining the property in the class.
##
nifi-nar-bundles/nifi-standard-services/nifi-hadoop-dbcp-service-bundle/nifi-hadoop-dbcp-service/src/main/java/org/apache/nifi/dbcp/HadoopDBCPConnectionPool.java:
##
@@ -86,37 +99,13 @@
)
}
)
-public class HadoopDBCPConnectionPool extends AbstractControllerService
implements DBCPService {
+public class HadoopDBCPConnectionPool extends AbstractDBCPConnectionPool {
private static final String ALLOW_EXPLICIT_KEYTAB =
"NIFI_ALLOW_EXPLICIT_KEYTAB";
private static final String HADOOP_CONFIGURATION_CLASS =
"org.apache.hadoop.conf.Configuration";
private static final String HADOOP_UGI_CLASS =
"org.apache.hadoop.security.UserGroupInformation";
-private static final String DEFAULT_MIN_IDLE = "0";
-private static final String DEFAULT_MAX_IDLE = "8";
-private static final String DEFAULT_MAX_CONN_LIFETIME = "-1";
-private static final String DEFAULT_EVICTION_RUN_PERIOD =
String.valueOf(-1L);
-private static final String DEFAULT_MIN_EVICTABLE_IDLE_TIME = "30 mins";
-private static final String DEFAULT_SOFT_MIN_EVICTABLE_IDLE_TIME =
String.valueOf(-1L);
-
-public static final PropertyDescriptor DATABASE_URL = new
PropertyDescriptor.Builder()
-.name("Database Connection URL")
-.description("A database connection URL used to connect to a
database. May contain database system name, host, port, database name and some
parameters."
-+ " The exact syntax of a database connection URL is
specified by your DBMS.")
-.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
-.required(true)
-.expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
-.build();
-
-public static final PropertyDescriptor DB_DRIVERNAME = new
PropertyDescriptor.Builder()
-.name("Database Driver Class Name")
-.description("Database driver class name")
-.required(true)
-.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
-.expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
-.build();
-
public static final PropertyDescriptor DB_DRIVER_LOCATION = new
PropertyDescriptor.Builder()
Review Comment:
It could be simplified via reusing and customizing the property descriptor
from `DBCPProperties`:
```
public static final PropertyDescriptor DB_DRIVER_LOCATION = new
PropertyDescriptor.Builder()
.fromPropertyDescriptor(DBCPProperties.DB_DRIVER_LOCATION)
.description("Comma-separated list of files/folders and/or URLs
containing the driver JAR and its dependencies (if any). " +
"For example '/var/tmp/phoenix-client.jar'. NOTE: It is
required that the resources specified by this property provide " +
"the classes from hadoop-common, such as Configuration
and UserGroupInformation.")
.required(true)
.build();
```
Also, the description says that some jars are required and the property is
also `required(true)`. So removing "_(if any)_" would make sense.
##
nifi-nar-bundles/nifi-standard-services/nifi-hadoop-dbcp-service-bundle/nifi-hadoop-dbcp-service/src/main/java/org/apache/nifi/dbcp/HadoopDBCPConnectionPool.java:
##
@@ -498,10 +365,10 @@ public void onEnabled(final ConfigurationContext context)
throws IOException {
if (resolvedKeytab != null) {
kerberosUser = new KerberosKeytabUser(resolvedPrincipal,
resolvedKeytab);
-getLogger().info("Security Enabled, logging in as principal {}
with keytab {}", new Object[] {resolvedPrincipal,
Re: [PR] NIFI-12890: Refactor HadoopDBCPConnectionPool to extend AbstractDBCPConnectionPool [nifi]
turcsanyip commented on code in PR #8619:
URL: https://github.com/apache/nifi/pull/8619#discussion_r1559652391
##
nifi-nar-bundles/nifi-standard-services/nifi-hadoop-dbcp-service-bundle/nifi-hadoop-dbcp-service/src/main/java/org/apache/nifi/dbcp/HadoopDBCPConnectionPool.java:
##
@@ -582,6 +409,41 @@ public void shutdown() throws SQLException {
}
}
+@Override
+protected Driver getDriver(String driverName, String url) {
+return null;
+}
+
+@Override
+protected DataSourceConfiguration
getDataSourceConfiguration(ConfigurationContext context) {
+final String url =
context.getProperty(DATABASE_URL).evaluateAttributeExpressions().getValue();
+final String driverName =
context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
+final String user =
context.getProperty(DB_USER).evaluateAttributeExpressions().getValue();
+final String password =
context.getProperty(DB_PASSWORD).evaluateAttributeExpressions().getValue();
+final Integer maxTotal =
context.getProperty(MAX_TOTAL_CONNECTIONS).evaluateAttributeExpressions().asInteger();
+final String validationQuery =
context.getProperty(VALIDATION_QUERY).evaluateAttributeExpressions().getValue();
+final Long maxWaitMillis =
extractMillisWithInfinite(context.getProperty(MAX_WAIT_TIME).evaluateAttributeExpressions());
+final Integer minIdle =
context.getProperty(MIN_IDLE).evaluateAttributeExpressions().asInteger();
+final Integer maxIdle =
context.getProperty(MAX_IDLE).evaluateAttributeExpressions().asInteger();
+final Long maxConnLifetimeMillis =
extractMillisWithInfinite(context.getProperty(MAX_CONN_LIFETIME).evaluateAttributeExpressions());
+final Long timeBetweenEvictionRunsMillis =
extractMillisWithInfinite(context.getProperty(EVICTION_RUN_PERIOD).evaluateAttributeExpressions());
+final Long minEvictableIdleTimeMillis =
extractMillisWithInfinite(context.getProperty(MIN_EVICTABLE_IDLE_TIME).evaluateAttributeExpressions());
+final Long softMinEvictableIdleTimeMillis =
extractMillisWithInfinite(context.getProperty(SOFT_MIN_EVICTABLE_IDLE_TIME).evaluateAttributeExpressions());
+
+return new DataSourceConfiguration.Builder(url, driverName, user,
password)
+.validationQuery(validationQuery)
+.driverClassLoader(this.getClass().getClassLoader())
Review Comment:
I see it comes from the old implementation but instead of setting the
`ClassLoader`, the `Driver` could be initialized in this class (similar to
[DBCPConnectionPool.getDriver()](https://github.com/apache/nifi/blob/90d0f6317a94d1730f779b719398c6a384fb9033/nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java#L288-L312)).
In this case, `AbstractDBCPConnectionPool` and `DataSourceConfiguration` do
not need to be changed.
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] NIFI-12890: Refactor HadoopDBCPConnectionPool to extend AbstractDBCPConnectionPool [nifi]
turcsanyip commented on PR #8619: URL: https://github.com/apache/nifi/pull/8619#issuecomment-2045964945 Reviewing... -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] NIFI-12890: Refactor HadoopDBCPConnectionPool to extend AbstractDBCPConnectionPool [nifi]
Lehel44 opened a new pull request, #8619: URL: https://github.com/apache/nifi/pull/8619 # Summary [NIFI-12890](https://issues.apache.org/jira/browse/NIFI-12890) # Tracking Please complete the following tracking steps prior to pull request creation. ### Issue Tracking - [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created ### Pull Request Tracking - [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-0` - [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-0` ### Pull Request Formatting - [ ] Pull Request based on current revision of the `main` branch - [ ] Pull Request refers to a feature branch with one commit containing changes # Verification Please indicate the verification steps performed prior to pull request creation. ### Build - [ ] Build completed using `mvn clean install -P contrib-check` - [ ] JDK 21 ### Licensing - [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html) - [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files ### Documentation - [ ] Documentation formatting appears as expected in rendered files -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
