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, resolvedKeytab});
+ getLogger().info("Security Enabled, logging in as principal {}
with keytab {}", new Object[]{resolvedPrincipal, resolvedKeytab});
Review Comment:
```suggestion
getLogger().info("Security Enabled, logging in as principal
{} with keytab {}", resolvedPrincipal, resolvedKeytab);
```
##########
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, resolvedKeytab});
+ getLogger().info("Security Enabled, logging in as principal {}
with keytab {}", new Object[]{resolvedPrincipal, resolvedKeytab});
} else if (explicitPassword != null) {
kerberosUser = new KerberosPasswordUser(resolvedPrincipal,
explicitPassword);
- getLogger().info("Security Enabled, logging in as principal {}
with password", new Object[] {resolvedPrincipal});
+ getLogger().info("Security Enabled, logging in as principal {}
with password", new Object[]{resolvedPrincipal});
Review Comment:
```suggestion
getLogger().info("Security Enabled, logging in as principal
{} with password", resolvedPrincipal);
```
--
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
