You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "Lehel44 (via GitHub)" <gi...@apache.org> on 2023/02/08 19:09:21 UTC
[GitHub] [nifi] Lehel44 opened a new pull request, #6935: NIFI-11151: Improving code reusability of DBCP services
Lehel44 opened a new pull request, #6935:
URL: https://github.com/apache/nifi/pull/6935
<!-- Licensed to the Apache Software Foundation (ASF) under one or more -->
<!-- contributor license agreements. See the NOTICE file distributed with -->
<!-- this work for additional information regarding copyright ownership. -->
<!-- The ASF licenses this file to You under the Apache License, Version 2.0 -->
<!-- (the "License"); you may not use this file except in compliance with -->
<!-- the License. You may obtain a copy of the License at -->
<!-- http://www.apache.org/licenses/LICENSE-2.0 -->
<!-- Unless required by applicable law or agreed to in writing, software -->
<!-- distributed under the License is distributed on an "AS IS" BASIS, -->
<!-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -->
<!-- See the License for the specific language governing permissions and -->
<!-- limitations under the License. -->
# Summary
[NIFI-11151](https://issues.apache.org/jira/browse/NIFI-11151)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI-11151) issue created
### Pull Request Tracking
- [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### 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 8
- [ ] JDK 11
- [ ] JDK 17
### 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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] Lehel44 commented on a diff in pull request #6935: NIFI-11151: Improving code reusability of DBCP services
Posted by "Lehel44 (via GitHub)" <gi...@apache.org>.
Lehel44 commented on code in PR #6935:
URL: https://github.com/apache/nifi/pull/6935#discussion_r1112255835
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/AbstractDBCPConnectionPool.java:
##########
@@ -365,168 +121,151 @@ public List<ConfigVerificationResult> verify(final ConfigurationContext context,
verificationLogger.error("Failed to shut down data source", e);
}
}
-
return results;
}
/**
* Configures connection pool by creating an instance of the
* {@link BasicDataSource} based on configuration provided with
* {@link ConfigurationContext}.
- *
+ * <p>
* This operation makes no guarantees that the actual connection could be
* made since the underlying system may still go off-line during normal
* operation of the connection pool.
*
- * @param context
- * the configuration context
- * @throws InitializationException
- * if unable to create a database connection
+ * @param context the configuration context
+ * @throws InitializationException if unable to create a database connection
*/
@OnEnabled
public void onConfigured(final ConfigurationContext context) throws InitializationException {
- kerberosUser = getKerberosUser(context);
dataSource = new BasicDataSource();
- configureDataSource(dataSource, kerberosUser, context);
+ kerberosUser = getKerberosUser(context);
+ loginKerberos(kerberosUser);
+ final DataSourceConfiguration configuration = createDataSourceConfiguraton(context);
+ configureDataSource(context, configuration);
}
- private void configureDataSource(final BasicDataSource dataSource, final KerberosUser kerberosUser,
- final ConfigurationContext context) throws InitializationException {
- final String dburl = getUrl(context);
-
- final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
- final String user = context.getProperty(DB_USER).evaluateAttributeExpressions().getValue();
- final String passw = 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());
-
+ private void loginKerberos(KerberosUser kerberosUser) throws InitializationException {
if (kerberosUser != null) {
try {
kerberosUser.login();
} catch (KerberosLoginException e) {
throw new InitializationException("Unable to authenticate Kerberos principal", e);
}
}
+ }
- dataSource.setDriver(getDriver(driverName, dburl));
- dataSource.setMaxWaitMillis(maxWaitMillis);
- dataSource.setMaxTotal(maxTotal);
- dataSource.setMinIdle(minIdle);
- dataSource.setMaxIdle(maxIdle);
- dataSource.setMaxConnLifetimeMillis(maxConnLifetimeMillis);
- dataSource.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
- dataSource.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
- dataSource.setSoftMinEvictableIdleTimeMillis(softMinEvictableIdleTimeMillis);
+ protected abstract Driver getDriver(final String driverName, final String url);
- if (validationQuery != null && !validationQuery.isEmpty()) {
+ protected DataSourceConfiguration createDataSourceConfiguraton(final ConfigurationContext context) {
+ final String url = context.getProperty(DBCPProperties.DATABASE_URL).getValue();
+ final String user = context.getProperty(DBCPProperties.DB_USER).getValue();
+ final String password = context.getProperty(DBCPProperties.DB_PASSWORD).getValue();
+ final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
+
+ return new DataSourceConfiguration.Builder(url, driverName, user, password)
+ .build();
+ }
Review Comment:
These four properties are the necessary ones and the optionals are set to default values. I'd assume those won't be modified very often. What do you think about adding a copy constructor to DataSourceConfiguration, so the extended classes could do something like `new DataSourceConfiguration(extendedConfig).Builder().optionalConfigValue1(12)...build()` ?
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6935: NIFI-11151: Improving code reusability of DBCP services
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #6935:
URL: https://github.com/apache/nifi/pull/6935#discussion_r1115047210
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/AbstractDBCPConnectionPool.java:
##########
@@ -365,168 +119,143 @@ public List<ConfigVerificationResult> verify(final ConfigurationContext context,
verificationLogger.error("Failed to shut down data source", e);
}
}
-
return results;
}
/**
* Configures connection pool by creating an instance of the
* {@link BasicDataSource} based on configuration provided with
* {@link ConfigurationContext}.
- *
+ * <p>
* This operation makes no guarantees that the actual connection could be
* made since the underlying system may still go off-line during normal
* operation of the connection pool.
*
- * @param context
- * the configuration context
- * @throws InitializationException
- * if unable to create a database connection
+ * @param context the configuration context
+ * @throws InitializationException if unable to create a database connection
*/
@OnEnabled
public void onConfigured(final ConfigurationContext context) throws InitializationException {
- kerberosUser = getKerberosUser(context);
dataSource = new BasicDataSource();
- configureDataSource(dataSource, kerberosUser, context);
+ kerberosUser = getKerberosUser(context);
+ loginKerberos(kerberosUser);
+ final DataSourceConfiguration configuration = getDataSourceConfigurationBuilder(context);
+ configureDataSource(context, configuration);
}
- private void configureDataSource(final BasicDataSource dataSource, final KerberosUser kerberosUser,
- final ConfigurationContext context) throws InitializationException {
- final String dburl = getUrl(context);
-
- final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
- final String user = context.getProperty(DB_USER).evaluateAttributeExpressions().getValue();
- final String passw = 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());
-
+ private void loginKerberos(KerberosUser kerberosUser) throws InitializationException {
if (kerberosUser != null) {
try {
kerberosUser.login();
} catch (KerberosLoginException e) {
throw new InitializationException("Unable to authenticate Kerberos principal", e);
}
}
+ }
+
+ protected abstract Driver getDriver(final String driverName, final String url);
- dataSource.setDriver(getDriver(driverName, dburl));
- dataSource.setMaxWaitMillis(maxWaitMillis);
- dataSource.setMaxTotal(maxTotal);
- dataSource.setMinIdle(minIdle);
- dataSource.setMaxIdle(maxIdle);
- dataSource.setMaxConnLifetimeMillis(maxConnLifetimeMillis);
- dataSource.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
- dataSource.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
- dataSource.setSoftMinEvictableIdleTimeMillis(softMinEvictableIdleTimeMillis);
+ protected abstract DataSourceConfiguration getDataSourceConfigurationBuilder(final ConfigurationContext context);
- if (validationQuery != null && !validationQuery.isEmpty()) {
+ protected void configureDataSource(final ConfigurationContext context, final DataSourceConfiguration configuration) {
+ final Driver driver = getDriver(configuration.getDriverName(), configuration.getUrl());
+
+ dataSource.setDriver(driver);
+ dataSource.setMaxWaitMillis(configuration.getMaxWaitMillis());
+ dataSource.setMaxTotal(configuration.getMaxTotal());
+ dataSource.setMinIdle(configuration.getMinIdle());
+ dataSource.setMaxIdle(configuration.getMaxIdle());
+ dataSource.setMaxConnLifetimeMillis(configuration.getMaxConnLifetimeMillis());
+ dataSource.setTimeBetweenEvictionRunsMillis(configuration.getTimeBetweenEvictionRunsMillis());
+ dataSource.setMinEvictableIdleTimeMillis(configuration.getMinEvictableIdleTimeMillis());
+ dataSource.setSoftMinEvictableIdleTimeMillis(configuration.getSoftMinEvictableIdleTimeMillis());
+
+ final String validationQuery = configuration.getValidationQuery();
+ if (StringUtils.isNotBlank(validationQuery)) {
dataSource.setValidationQuery(validationQuery);
dataSource.setTestOnBorrow(true);
}
- dataSource.setUrl(dburl);
- dataSource.setUsername(user);
- dataSource.setPassword(passw);
+ dataSource.setUrl(configuration.getUrl());
+ dataSource.setUsername(configuration.getUserName());
+ dataSource.setPassword(configuration.getPassword());
+
+ getConnectionProperties(context).forEach(dataSource::addConnectionProperty);
+ }
- final List<PropertyDescriptor> dynamicProperties = context.getProperties()
+ protected Map<String, String> getConnectionProperties(final ConfigurationContext context) {
+ return getDynamicProperties(context)
+ .stream()
+ .collect(Collectors.toMap(PropertyDescriptor::getName, s -> {
+ final PropertyValue propertyValue = context.getProperty(s);
+ return propertyValue.evaluateAttributeExpressions().getValue();
+ }));
+ }
+
+ protected List<PropertyDescriptor> getDynamicProperties(final ConfigurationContext context) {
+ return context.getProperties()
.keySet()
.stream()
.filter(PropertyDescriptor::isDynamic)
.collect(Collectors.toList());
-
- dynamicProperties.forEach((descriptor) -> {
- final PropertyValue propertyValue = context.getProperty(descriptor);
- if (descriptor.isSensitive()) {
- final String propertyName = StringUtils.substringAfter(descriptor.getName(), SENSITIVE_PROPERTY_PREFIX);
- dataSource.addConnectionProperty(propertyName, propertyValue.getValue());
- } else {
- dataSource.addConnectionProperty(descriptor.getName(), propertyValue.evaluateAttributeExpressions().getValue());
- }
- });
-
- getConnectionProperties(context).forEach(dataSource::addConnectionProperty);
}
- private KerberosUser getKerberosUser(final ConfigurationContext context) {
- KerberosUser kerberosUser = null;
- final KerberosCredentialsService kerberosCredentialsService = context.getProperty(KERBEROS_CREDENTIALS_SERVICE).asControllerService(KerberosCredentialsService.class);
+ protected KerberosUser getKerberosUser(final ConfigurationContext context) {
+ final KerberosUser kerberosUser;
final KerberosUserService kerberosUserService = context.getProperty(KERBEROS_USER_SERVICE).asControllerService(KerberosUserService.class);
- final String kerberosPrincipal = context.getProperty(KERBEROS_PRINCIPAL).evaluateAttributeExpressions().getValue();
- final String kerberosPassword = context.getProperty(KERBEROS_PASSWORD).getValue();
if (kerberosUserService != null) {
kerberosUser = kerberosUserService.createKerberosUser();
- } else if (kerberosCredentialsService != null) {
- kerberosUser = new KerberosKeytabUser(kerberosCredentialsService.getPrincipal(), kerberosCredentialsService.getKeytab());
- } else if (!StringUtils.isBlank(kerberosPrincipal) && !StringUtils.isBlank(kerberosPassword)) {
- kerberosUser = new KerberosPasswordUser(kerberosPrincipal, kerberosPassword);
+ } else {
+ kerberosUser = getKerberosUserByCredentials(context);
}
return kerberosUser;
}
- protected String getUrl(ConfigurationContext context) {
- return context.getProperty(DATABASE_URL).evaluateAttributeExpressions().getValue();
+ protected KerberosUser getKerberosUserByCredentials(final ConfigurationContext context) {
+ return null;
}
Review Comment:
Instead of having this method implementation return `null` in this abstract class, it looks like it could be removed, and the concrete implementation could override the `getKerberosUser()` method.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] Lehel44 commented on a diff in pull request #6935: NIFI-11151: Improving code reusability of DBCP services
Posted by "Lehel44 (via GitHub)" <gi...@apache.org>.
Lehel44 commented on code in PR #6935:
URL: https://github.com/apache/nifi/pull/6935#discussion_r1112304213
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/AbstractDBCPConnectionPool.java:
##########
@@ -365,168 +121,151 @@ public List<ConfigVerificationResult> verify(final ConfigurationContext context,
verificationLogger.error("Failed to shut down data source", e);
}
}
-
return results;
}
/**
* Configures connection pool by creating an instance of the
* {@link BasicDataSource} based on configuration provided with
* {@link ConfigurationContext}.
- *
+ * <p>
* This operation makes no guarantees that the actual connection could be
* made since the underlying system may still go off-line during normal
* operation of the connection pool.
*
- * @param context
- * the configuration context
- * @throws InitializationException
- * if unable to create a database connection
+ * @param context the configuration context
+ * @throws InitializationException if unable to create a database connection
*/
@OnEnabled
public void onConfigured(final ConfigurationContext context) throws InitializationException {
- kerberosUser = getKerberosUser(context);
dataSource = new BasicDataSource();
- configureDataSource(dataSource, kerberosUser, context);
+ kerberosUser = getKerberosUser(context);
+ loginKerberos(kerberosUser);
+ final DataSourceConfiguration configuration = createDataSourceConfiguraton(context);
+ configureDataSource(context, configuration);
}
- private void configureDataSource(final BasicDataSource dataSource, final KerberosUser kerberosUser,
- final ConfigurationContext context) throws InitializationException {
- final String dburl = getUrl(context);
-
- final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
- final String user = context.getProperty(DB_USER).evaluateAttributeExpressions().getValue();
- final String passw = 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());
-
+ private void loginKerberos(KerberosUser kerberosUser) throws InitializationException {
if (kerberosUser != null) {
try {
kerberosUser.login();
} catch (KerberosLoginException e) {
throw new InitializationException("Unable to authenticate Kerberos principal", e);
}
}
+ }
- dataSource.setDriver(getDriver(driverName, dburl));
- dataSource.setMaxWaitMillis(maxWaitMillis);
- dataSource.setMaxTotal(maxTotal);
- dataSource.setMinIdle(minIdle);
- dataSource.setMaxIdle(maxIdle);
- dataSource.setMaxConnLifetimeMillis(maxConnLifetimeMillis);
- dataSource.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
- dataSource.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
- dataSource.setSoftMinEvictableIdleTimeMillis(softMinEvictableIdleTimeMillis);
+ protected abstract Driver getDriver(final String driverName, final String url);
- if (validationQuery != null && !validationQuery.isEmpty()) {
+ protected DataSourceConfiguration createDataSourceConfiguraton(final ConfigurationContext context) {
+ final String url = context.getProperty(DBCPProperties.DATABASE_URL).getValue();
+ final String user = context.getProperty(DBCPProperties.DB_USER).getValue();
+ final String password = context.getProperty(DBCPProperties.DB_PASSWORD).getValue();
+ final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
+
+ return new DataSourceConfiguration.Builder(url, driverName, user, password)
+ .build();
+ }
Review Comment:
I agree that it is less reusable. The problem with returning with builder is that the mandatory fields can't be changed afterwards. The url is mandatory and is overwritten by several descendant classes. You could override a getUrl method, but I think it would be an overdesign, so I would just make the method plain abstract.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] Lehel44 commented on a diff in pull request #6935: NIFI-11151: Improving code reusability of DBCP services
Posted by "Lehel44 (via GitHub)" <gi...@apache.org>.
Lehel44 commented on code in PR #6935:
URL: https://github.com/apache/nifi/pull/6935#discussion_r1112299773
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/AbstractDBCPConnectionPool.java:
##########
@@ -365,168 +121,151 @@ public List<ConfigVerificationResult> verify(final ConfigurationContext context,
verificationLogger.error("Failed to shut down data source", e);
}
}
-
return results;
}
/**
* Configures connection pool by creating an instance of the
* {@link BasicDataSource} based on configuration provided with
* {@link ConfigurationContext}.
- *
+ * <p>
* This operation makes no guarantees that the actual connection could be
* made since the underlying system may still go off-line during normal
* operation of the connection pool.
*
- * @param context
- * the configuration context
- * @throws InitializationException
- * if unable to create a database connection
+ * @param context the configuration context
+ * @throws InitializationException if unable to create a database connection
*/
@OnEnabled
public void onConfigured(final ConfigurationContext context) throws InitializationException {
- kerberosUser = getKerberosUser(context);
dataSource = new BasicDataSource();
- configureDataSource(dataSource, kerberosUser, context);
+ kerberosUser = getKerberosUser(context);
+ loginKerberos(kerberosUser);
+ final DataSourceConfiguration configuration = createDataSourceConfiguraton(context);
+ configureDataSource(context, configuration);
}
- private void configureDataSource(final BasicDataSource dataSource, final KerberosUser kerberosUser,
- final ConfigurationContext context) throws InitializationException {
- final String dburl = getUrl(context);
-
- final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
- final String user = context.getProperty(DB_USER).evaluateAttributeExpressions().getValue();
- final String passw = 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());
-
+ private void loginKerberos(KerberosUser kerberosUser) throws InitializationException {
if (kerberosUser != null) {
try {
kerberosUser.login();
} catch (KerberosLoginException e) {
throw new InitializationException("Unable to authenticate Kerberos principal", e);
}
}
+ }
- dataSource.setDriver(getDriver(driverName, dburl));
- dataSource.setMaxWaitMillis(maxWaitMillis);
- dataSource.setMaxTotal(maxTotal);
- dataSource.setMinIdle(minIdle);
- dataSource.setMaxIdle(maxIdle);
- dataSource.setMaxConnLifetimeMillis(maxConnLifetimeMillis);
- dataSource.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
- dataSource.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
- dataSource.setSoftMinEvictableIdleTimeMillis(softMinEvictableIdleTimeMillis);
+ protected abstract Driver getDriver(final String driverName, final String url);
- if (validationQuery != null && !validationQuery.isEmpty()) {
+ protected DataSourceConfiguration createDataSourceConfiguraton(final ConfigurationContext context) {
+ final String url = context.getProperty(DBCPProperties.DATABASE_URL).getValue();
+ final String user = context.getProperty(DBCPProperties.DB_USER).getValue();
+ final String password = context.getProperty(DBCPProperties.DB_PASSWORD).getValue();
+ final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
+
+ return new DataSourceConfiguration.Builder(url, driverName, user, password)
+ .build();
+ }
Review Comment:
This way we can't override the builder's contructor. Some child classes modifies the url for example which is a required builder field. I think a copy constructor would work.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] Lehel44 commented on a diff in pull request #6935: NIFI-11151: Improving code reusability of DBCP services
Posted by "Lehel44 (via GitHub)" <gi...@apache.org>.
Lehel44 commented on code in PR #6935:
URL: https://github.com/apache/nifi/pull/6935#discussion_r1112299773
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/AbstractDBCPConnectionPool.java:
##########
@@ -365,168 +121,151 @@ public List<ConfigVerificationResult> verify(final ConfigurationContext context,
verificationLogger.error("Failed to shut down data source", e);
}
}
-
return results;
}
/**
* Configures connection pool by creating an instance of the
* {@link BasicDataSource} based on configuration provided with
* {@link ConfigurationContext}.
- *
+ * <p>
* This operation makes no guarantees that the actual connection could be
* made since the underlying system may still go off-line during normal
* operation of the connection pool.
*
- * @param context
- * the configuration context
- * @throws InitializationException
- * if unable to create a database connection
+ * @param context the configuration context
+ * @throws InitializationException if unable to create a database connection
*/
@OnEnabled
public void onConfigured(final ConfigurationContext context) throws InitializationException {
- kerberosUser = getKerberosUser(context);
dataSource = new BasicDataSource();
- configureDataSource(dataSource, kerberosUser, context);
+ kerberosUser = getKerberosUser(context);
+ loginKerberos(kerberosUser);
+ final DataSourceConfiguration configuration = createDataSourceConfiguraton(context);
+ configureDataSource(context, configuration);
}
- private void configureDataSource(final BasicDataSource dataSource, final KerberosUser kerberosUser,
- final ConfigurationContext context) throws InitializationException {
- final String dburl = getUrl(context);
-
- final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
- final String user = context.getProperty(DB_USER).evaluateAttributeExpressions().getValue();
- final String passw = 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());
-
+ private void loginKerberos(KerberosUser kerberosUser) throws InitializationException {
if (kerberosUser != null) {
try {
kerberosUser.login();
} catch (KerberosLoginException e) {
throw new InitializationException("Unable to authenticate Kerberos principal", e);
}
}
+ }
- dataSource.setDriver(getDriver(driverName, dburl));
- dataSource.setMaxWaitMillis(maxWaitMillis);
- dataSource.setMaxTotal(maxTotal);
- dataSource.setMinIdle(minIdle);
- dataSource.setMaxIdle(maxIdle);
- dataSource.setMaxConnLifetimeMillis(maxConnLifetimeMillis);
- dataSource.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
- dataSource.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
- dataSource.setSoftMinEvictableIdleTimeMillis(softMinEvictableIdleTimeMillis);
+ protected abstract Driver getDriver(final String driverName, final String url);
- if (validationQuery != null && !validationQuery.isEmpty()) {
+ protected DataSourceConfiguration createDataSourceConfiguraton(final ConfigurationContext context) {
+ final String url = context.getProperty(DBCPProperties.DATABASE_URL).getValue();
+ final String user = context.getProperty(DBCPProperties.DB_USER).getValue();
+ final String password = context.getProperty(DBCPProperties.DB_PASSWORD).getValue();
+ final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
+
+ return new DataSourceConfiguration.Builder(url, driverName, user, password)
+ .build();
+ }
Review Comment:
We can't override the builder's contructor this way. Some child classes, for example, change the url, which is a necessary constructor field. I believe a copy constructor would make sense. Maybe the url might be overridden as well, as there appears to be a requirement for flexibility.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] asfgit closed pull request #6935: NIFI-11151: Improving code reusability of DBCP services
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #6935: NIFI-11151: Improving code reusability of DBCP services
URL: https://github.com/apache/nifi/pull/6935
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] turcsanyip commented on a diff in pull request #6935: NIFI-11151: Improving code reusability of DBCP services
Posted by "turcsanyip (via GitHub)" <gi...@apache.org>.
turcsanyip commented on code in PR #6935:
URL: https://github.com/apache/nifi/pull/6935#discussion_r1127644882
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/AbstractDBCPConnectionPool.java:
##########
@@ -365,168 +119,136 @@ public List<ConfigVerificationResult> verify(final ConfigurationContext context,
verificationLogger.error("Failed to shut down data source", e);
}
}
-
return results;
}
/**
* Configures connection pool by creating an instance of the
* {@link BasicDataSource} based on configuration provided with
* {@link ConfigurationContext}.
- *
+ * <p>
* This operation makes no guarantees that the actual connection could be
* made since the underlying system may still go off-line during normal
* operation of the connection pool.
*
- * @param context
- * the configuration context
- * @throws InitializationException
- * if unable to create a database connection
+ * @param context the configuration context
+ * @throws InitializationException if unable to create a database connection
*/
@OnEnabled
public void onConfigured(final ConfigurationContext context) throws InitializationException {
- kerberosUser = getKerberosUser(context);
dataSource = new BasicDataSource();
- configureDataSource(dataSource, kerberosUser, context);
+ kerberosUser = getKerberosUser(context);
+ loginKerberos(kerberosUser);
+ final DataSourceConfiguration configuration = getDataSourceConfigurationBuilder(context);
+ configureDataSource(context, configuration);
}
- private void configureDataSource(final BasicDataSource dataSource, final KerberosUser kerberosUser,
- final ConfigurationContext context) throws InitializationException {
- final String dburl = getUrl(context);
-
- final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
- final String user = context.getProperty(DB_USER).evaluateAttributeExpressions().getValue();
- final String passw = 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());
-
+ private void loginKerberos(KerberosUser kerberosUser) throws InitializationException {
if (kerberosUser != null) {
try {
kerberosUser.login();
} catch (KerberosLoginException e) {
throw new InitializationException("Unable to authenticate Kerberos principal", e);
}
}
+ }
+
+ protected abstract Driver getDriver(final String driverName, final String url);
- dataSource.setDriver(getDriver(driverName, dburl));
- dataSource.setMaxWaitMillis(maxWaitMillis);
- dataSource.setMaxTotal(maxTotal);
- dataSource.setMinIdle(minIdle);
- dataSource.setMaxIdle(maxIdle);
- dataSource.setMaxConnLifetimeMillis(maxConnLifetimeMillis);
- dataSource.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
- dataSource.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
- dataSource.setSoftMinEvictableIdleTimeMillis(softMinEvictableIdleTimeMillis);
+ protected abstract DataSourceConfiguration getDataSourceConfigurationBuilder(final ConfigurationContext context);
Review Comment:
It does not return a builder but the configuration object itself.
```suggestion
protected abstract DataSourceConfiguration getDataSourceConfiguration(final ConfigurationContext context);
```
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] Lehel44 commented on pull request #6935: NIFI-11151: Improving code reusability of DBCP services
Posted by "Lehel44 (via GitHub)" <gi...@apache.org>.
Lehel44 commented on PR #6935:
URL: https://github.com/apache/nifi/pull/6935#issuecomment-1431523294
Thanks for the related reviews to this PR @exceptionfactory @turcsanyip. With a more general approach I refactored the AbstractDBCPConnectionPool to be more generic and flexible.
- I moved the PropertyDescriptors to a different class to avoid unnecessary inheritance.
- I kept parts that is currently being reused and can be reused in the abstract super class
- Moved domain specific code to the extending subclasses
- Created DataSourceConfiguration to decouple PropertyDescriptors and DataSource configuration.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] turcsanyip commented on a diff in pull request #6935: NIFI-11151: Improving code reusability of DBCP services
Posted by "turcsanyip (via GitHub)" <gi...@apache.org>.
turcsanyip commented on code in PR #6935:
URL: https://github.com/apache/nifi/pull/6935#discussion_r1112089266
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/AbstractDBCPConnectionPool.java:
##########
@@ -365,168 +121,151 @@ public List<ConfigVerificationResult> verify(final ConfigurationContext context,
verificationLogger.error("Failed to shut down data source", e);
}
}
-
return results;
}
/**
* Configures connection pool by creating an instance of the
* {@link BasicDataSource} based on configuration provided with
* {@link ConfigurationContext}.
- *
+ * <p>
* This operation makes no guarantees that the actual connection could be
* made since the underlying system may still go off-line during normal
* operation of the connection pool.
*
- * @param context
- * the configuration context
- * @throws InitializationException
- * if unable to create a database connection
+ * @param context the configuration context
+ * @throws InitializationException if unable to create a database connection
*/
@OnEnabled
public void onConfigured(final ConfigurationContext context) throws InitializationException {
- kerberosUser = getKerberosUser(context);
dataSource = new BasicDataSource();
- configureDataSource(dataSource, kerberosUser, context);
+ kerberosUser = getKerberosUser(context);
+ loginKerberos(kerberosUser);
+ final DataSourceConfiguration configuration = createDataSourceConfiguraton(context);
+ configureDataSource(context, configuration);
}
- private void configureDataSource(final BasicDataSource dataSource, final KerberosUser kerberosUser,
- final ConfigurationContext context) throws InitializationException {
- final String dburl = getUrl(context);
-
- final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
- final String user = context.getProperty(DB_USER).evaluateAttributeExpressions().getValue();
- final String passw = 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());
-
+ private void loginKerberos(KerberosUser kerberosUser) throws InitializationException {
if (kerberosUser != null) {
try {
kerberosUser.login();
} catch (KerberosLoginException e) {
throw new InitializationException("Unable to authenticate Kerberos principal", e);
}
}
+ }
- dataSource.setDriver(getDriver(driverName, dburl));
- dataSource.setMaxWaitMillis(maxWaitMillis);
- dataSource.setMaxTotal(maxTotal);
- dataSource.setMinIdle(minIdle);
- dataSource.setMaxIdle(maxIdle);
- dataSource.setMaxConnLifetimeMillis(maxConnLifetimeMillis);
- dataSource.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
- dataSource.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
- dataSource.setSoftMinEvictableIdleTimeMillis(softMinEvictableIdleTimeMillis);
+ protected abstract Driver getDriver(final String driverName, final String url);
- if (validationQuery != null && !validationQuery.isEmpty()) {
+ protected DataSourceConfiguration createDataSourceConfiguraton(final ConfigurationContext context) {
Review Comment:
Typo:
```suggestion
protected DataSourceConfiguration createDataSourceConfiguration(final ConfigurationContext context) {
```
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/AbstractDBCPConnectionPool.java:
##########
@@ -365,168 +121,151 @@ public List<ConfigVerificationResult> verify(final ConfigurationContext context,
verificationLogger.error("Failed to shut down data source", e);
}
}
-
return results;
}
/**
* Configures connection pool by creating an instance of the
* {@link BasicDataSource} based on configuration provided with
* {@link ConfigurationContext}.
- *
+ * <p>
* This operation makes no guarantees that the actual connection could be
* made since the underlying system may still go off-line during normal
* operation of the connection pool.
*
- * @param context
- * the configuration context
- * @throws InitializationException
- * if unable to create a database connection
+ * @param context the configuration context
+ * @throws InitializationException if unable to create a database connection
*/
@OnEnabled
public void onConfigured(final ConfigurationContext context) throws InitializationException {
- kerberosUser = getKerberosUser(context);
dataSource = new BasicDataSource();
- configureDataSource(dataSource, kerberosUser, context);
+ kerberosUser = getKerberosUser(context);
+ loginKerberos(kerberosUser);
+ final DataSourceConfiguration configuration = createDataSourceConfiguraton(context);
+ configureDataSource(context, configuration);
}
- private void configureDataSource(final BasicDataSource dataSource, final KerberosUser kerberosUser,
- final ConfigurationContext context) throws InitializationException {
- final String dburl = getUrl(context);
-
- final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
- final String user = context.getProperty(DB_USER).evaluateAttributeExpressions().getValue();
- final String passw = 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());
-
+ private void loginKerberos(KerberosUser kerberosUser) throws InitializationException {
if (kerberosUser != null) {
try {
kerberosUser.login();
} catch (KerberosLoginException e) {
throw new InitializationException("Unable to authenticate Kerberos principal", e);
}
}
+ }
- dataSource.setDriver(getDriver(driverName, dburl));
- dataSource.setMaxWaitMillis(maxWaitMillis);
- dataSource.setMaxTotal(maxTotal);
- dataSource.setMinIdle(minIdle);
- dataSource.setMaxIdle(maxIdle);
- dataSource.setMaxConnLifetimeMillis(maxConnLifetimeMillis);
- dataSource.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
- dataSource.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
- dataSource.setSoftMinEvictableIdleTimeMillis(softMinEvictableIdleTimeMillis);
+ protected abstract Driver getDriver(final String driverName, final String url);
- if (validationQuery != null && !validationQuery.isEmpty()) {
+ protected DataSourceConfiguration createDataSourceConfiguraton(final ConfigurationContext context) {
+ final String url = context.getProperty(DBCPProperties.DATABASE_URL).getValue();
+ final String user = context.getProperty(DBCPProperties.DB_USER).getValue();
+ final String password = context.getProperty(DBCPProperties.DB_PASSWORD).getValue();
+ final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
+
+ return new DataSourceConfiguration.Builder(url, driverName, user, password)
+ .build();
+ }
+
+ protected void configureDataSource(final ConfigurationContext context, final DataSourceConfiguration configuration) {
+ final Driver driver = getDriver(configuration.getDriverName(), configuration.getUrl());
+
+ dataSource.setDriver(driver);
+ dataSource.setMaxWaitMillis(configuration.getMaxWaitMillis());
+ dataSource.setMaxTotal(configuration.getMaxTotal());
+ dataSource.setMinIdle(configuration.getMinIdle());
+ dataSource.setMaxIdle(configuration.getMaxIdle());
+ dataSource.setMaxConnLifetimeMillis(configuration.getMaxConnLifetimeMillis());
+ dataSource.setTimeBetweenEvictionRunsMillis(configuration.getTimeBetweenEvictionRunsMillis());
+ dataSource.setMinEvictableIdleTimeMillis(configuration.getMinEvictableIdleTimeMillis());
+ dataSource.setSoftMinEvictableIdleTimeMillis(configuration.getSoftMinEvictableIdleTimeMillis());
+
+ final String validationQuery = configuration.getValidationQuery();
+ if (StringUtils.isNotBlank(validationQuery)) {
dataSource.setValidationQuery(validationQuery);
dataSource.setTestOnBorrow(true);
}
- dataSource.setUrl(dburl);
- dataSource.setUsername(user);
- dataSource.setPassword(passw);
+ dataSource.setUrl(configuration.getUrl());
+ dataSource.setUsername(configuration.getUserName());
+ dataSource.setPassword(configuration.getPassword());
+
+ getConnectionProperties(context).forEach(dataSource::addConnectionProperty);
+ }
+
+ protected Map<String, String> getConnectionProperties(final ConfigurationContext context) {
+ return getDynamicProperties(context)
+ .stream()
+ .collect(Collectors.toMap(PropertyDescriptor::getName, s -> {
+ final PropertyValue propertyValue = context.getProperty(s);
+ return propertyValue.evaluateAttributeExpressions().getValue();
+ }));
+ }
- final List<PropertyDescriptor> dynamicProperties = context.getProperties()
+ protected List<PropertyDescriptor> getDynamicProperties(final ConfigurationContext context) {
+ return context.getProperties()
.keySet()
.stream()
.filter(PropertyDescriptor::isDynamic)
.collect(Collectors.toList());
-
- dynamicProperties.forEach((descriptor) -> {
- final PropertyValue propertyValue = context.getProperty(descriptor);
- if (descriptor.isSensitive()) {
- final String propertyName = StringUtils.substringAfter(descriptor.getName(), SENSITIVE_PROPERTY_PREFIX);
- dataSource.addConnectionProperty(propertyName, propertyValue.getValue());
- } else {
- dataSource.addConnectionProperty(descriptor.getName(), propertyValue.evaluateAttributeExpressions().getValue());
- }
- });
-
- getConnectionProperties(context).forEach(dataSource::addConnectionProperty);
}
- private KerberosUser getKerberosUser(final ConfigurationContext context) {
- KerberosUser kerberosUser = null;
- final KerberosCredentialsService kerberosCredentialsService = context.getProperty(KERBEROS_CREDENTIALS_SERVICE).asControllerService(KerberosCredentialsService.class);
+ protected KerberosUser getKerberosUser(final ConfigurationContext context) {
+ KerberosUser kerberosUser;
Review Comment:
```suggestion
final KerberosUser kerberosUser;
```
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/utils/DBCPProperties.java:
##########
@@ -0,0 +1,228 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.dbcp.utils;
+
+import org.apache.nifi.components.PropertyDescriptor;
+import org.apache.nifi.components.PropertyValue;
+import org.apache.nifi.components.resource.ResourceCardinality;
+import org.apache.nifi.components.resource.ResourceType;
+import org.apache.nifi.dbcp.DBCPValidator;
+import org.apache.nifi.expression.AttributeExpression;
+import org.apache.nifi.expression.ExpressionLanguageScope;
+import org.apache.nifi.kerberos.KerberosCredentialsService;
+import org.apache.nifi.kerberos.KerberosUserService;
+import org.apache.nifi.processor.util.StandardValidators;
+
+import java.util.concurrent.TimeUnit;
+
+public final class DBCPProperties {
+
+ private DBCPProperties() {
+ }
+
+ 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.")
+ .defaultValue(null)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .required(true)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
+
+ public static final PropertyDescriptor DB_USER = new PropertyDescriptor.Builder()
+ .name("Database User")
+ .description("Database user name")
+ .defaultValue(null)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
+
+ public static final PropertyDescriptor DB_PASSWORD = new PropertyDescriptor.Builder()
+ .name("Password")
+ .description("The password for the database user")
+ .defaultValue(null)
+ .required(false)
+ .sensitive(true)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
+
+
+ public static final PropertyDescriptor DB_DRIVERNAME = new PropertyDescriptor.Builder()
+ .name("Database Driver Class Name")
+ .description("Database driver class name")
+ .defaultValue(null)
+ .required(true)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
+
+ public static final PropertyDescriptor DB_DRIVER_LOCATION = new PropertyDescriptor.Builder()
+ .name("database-driver-locations")
+ .displayName("Database Driver Location(s)")
+ .description("Comma-separated list of files/folders and/or URLs containing the driver JAR and its dependencies (if any). For example '/var/tmp/mariadb-java-client-1.1.7.jar'")
+ .defaultValue(null)
+ .required(false)
+ .identifiesExternalResource(ResourceCardinality.MULTIPLE, ResourceType.FILE, ResourceType.DIRECTORY, ResourceType.URL)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .dynamicallyModifiesClasspath(true)
+ .build();
+
+ public static final PropertyDescriptor MAX_WAIT_TIME = new PropertyDescriptor.Builder()
+ .name("Max Wait Time")
+ .description("The maximum amount of time that the pool will wait (when there are no available connections) "
+ + " for a connection to be returned before failing, or -1 to wait indefinitely. ")
+ .defaultValue(DefaultDataSourceValues.MAX_WAIT_MILLIS.getPropertyValue())
+ .required(true)
+ .addValidator(DBCPValidator.CUSTOM_TIME_PERIOD_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .sensitive(false)
+ .build();
+
+ public static final PropertyDescriptor MAX_TOTAL_CONNECTIONS = new PropertyDescriptor.Builder()
+ .name("Max Total Connections")
+ .description("The maximum number of active connections that can be allocated from this pool at the same time, "
+ + " or negative for no limit.")
+ .defaultValue(DefaultDataSourceValues.MAX_TOTAL_CONNECTIONS.getPropertyValue())
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .sensitive(false)
+ .build();
+
+ public static final PropertyDescriptor VALIDATION_QUERY = new PropertyDescriptor.Builder()
+ .name("Validation-query")
+ .displayName("Validation query")
+ .description("Validation query used to validate connections before returning them. "
+ + "When connection is invalid, it gets dropped and new valid connection will be returned. "
+ + "Note!! Using validation might have some performance penalty.")
+ .required(false)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
+
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .displayName("Minimum Idle Connections")
+ .name("dbcp-min-idle-conns")
+ .description("The minimum number of connections that can remain idle in the pool without extra ones being " +
+ "created. Set to or zero to allow no idle connections.")
+ .defaultValue(DefaultDataSourceValues.MIN_IDLE.getPropertyValue())
+ .required(false)
+ .addValidator(StandardValidators.NON_NEGATIVE_INTEGER_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
+
+ public static final PropertyDescriptor MAX_IDLE = new PropertyDescriptor.Builder()
+ .displayName("Max Idle Connections")
+ .name("dbcp-max-idle-conns")
+ .description("The maximum number of connections that can remain idle in the pool without extra ones being " +
+ "released. Set to any negative value to allow unlimited idle connections.")
+ .defaultValue(DefaultDataSourceValues.MAX_IDLE.getPropertyValue())
+ .required(false)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
+
+ public static final PropertyDescriptor MAX_CONN_LIFETIME = new PropertyDescriptor.Builder()
+ .displayName("Max Connection Lifetime")
+ .name("dbcp-max-conn-lifetime")
+ .description("The maximum lifetime in milliseconds of a connection. After this time is exceeded the " +
+ "connection will fail the next activation, passivation or validation test. A value of zero or less " +
+ "means the connection has an infinite lifetime.")
+ .defaultValue(DefaultDataSourceValues.MAX_CONN_LIFETIME.getPropertyValue())
+ .required(false)
+ .addValidator(DBCPValidator.CUSTOM_TIME_PERIOD_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
+
+ public static final PropertyDescriptor EVICTION_RUN_PERIOD = new PropertyDescriptor.Builder()
+ .displayName("Time Between Eviction Runs")
+ .name("dbcp-time-between-eviction-runs")
+ .description("The number of milliseconds to sleep between runs of the idle connection evictor thread. When " +
+ "non-positive, no idle connection evictor thread will be run.")
+ .defaultValue(DefaultDataSourceValues.EVICTION_RUN_PERIOD.getPropertyValue())
+ .required(false)
+ .addValidator(DBCPValidator.CUSTOM_TIME_PERIOD_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
+
+ public static final PropertyDescriptor MIN_EVICTABLE_IDLE_TIME = new PropertyDescriptor.Builder()
+ .displayName("Minimum Evictable Idle Time")
+ .name("dbcp-min-evictable-idle-time")
+ .description("The minimum amount of time a connection may sit idle in the pool before it is eligible for eviction.")
+ .defaultValue(DefaultDataSourceValues.MIN_EVICTABLE_IDLE_TIME_MILLIS.getPropertyValue())
+ .required(false)
+ .addValidator(DBCPValidator.CUSTOM_TIME_PERIOD_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
+
+ public static final PropertyDescriptor SOFT_MIN_EVICTABLE_IDLE_TIME = new PropertyDescriptor.Builder()
+ .displayName("Soft Minimum Evictable Idle Time")
+ .name("dbcp-soft-min-evictable-idle-time")
+ .description("The minimum amount of time a connection may sit idle in the pool before it is eligible for " +
+ "eviction by the idle connection evictor, with the extra condition that at least a minimum number of" +
+ " idle connections remain in the pool. When the not-soft version of this option is set to a positive" +
+ " value, it is examined first by the idle connection evictor: when idle connections are visited by " +
+ "the evictor, idle time is first compared against it (without considering the number of idle " +
+ "connections in the pool) and then against this soft option, including the minimum idle connections " +
+ "constraint.")
+ .defaultValue(DefaultDataSourceValues.SOFT_MIN_EVICTABLE_IDLE_TIME.getPropertyValue())
+ .required(false)
+ .addValidator(DBCPValidator.CUSTOM_TIME_PERIOD_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .build();
+
+ public static final PropertyDescriptor KERBEROS_CREDENTIALS_SERVICE = new PropertyDescriptor.Builder()
Review Comment:
The legacy kerberos properties (`KERBEROS_CREDENTIALS_SERVICE`, `KERBEROS_PRINCIPAL`, `KERBEROS_PASSWORD`) should be moved to the existing processor(s) where they are in still use. This common code should only contain the current kerberos property / code.
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/utils/DefaultDataSourceValues.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.dbcp.utils;
+
+import org.apache.commons.dbcp2.BasicDataSource;
+import org.apache.commons.pool2.impl.GenericObjectPoolConfig;
+
+public enum DefaultDataSourceValues {
+
+
+ MAX_WAIT_MILLIS("500", "500 millis"),
Review Comment:
I think it is not necessary to define the same value in 2 formats because the millisec value could be calculated from the property value using `FormatUtils.getTimeDuration()` (the -1 special value needs to be handled too).
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/utils/DefaultDataSourceValues.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.dbcp.utils;
+
+import org.apache.commons.dbcp2.BasicDataSource;
+import org.apache.commons.pool2.impl.GenericObjectPoolConfig;
+
+public enum DefaultDataSourceValues {
+
+
+ MAX_WAIT_MILLIS("500", "500 millis"),
Review Comment:
```suggestion
MAX_WAIT_TIME("500", "500 millis"),
```
Other constants (`MAX_CONN_LIFETIME`, `EVICTION_RUN_PERIOD`) do not have `_MILLIS` suffix which I think is correct because the time period can be defined in different time units.
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/utils/DefaultDataSourceValues.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.dbcp.utils;
+
+import org.apache.commons.dbcp2.BasicDataSource;
+import org.apache.commons.pool2.impl.GenericObjectPoolConfig;
+
+public enum DefaultDataSourceValues {
+
+
+ MAX_WAIT_MILLIS("500", "500 millis"),
+
+ MAX_TOTAL_CONNECTIONS("8", "8"),
+ /**
+ * Copied from {@link GenericObjectPoolConfig#DEFAULT_MIN_IDLE} in Commons-DBCP 2.7.0
+ */
+ MIN_IDLE("0", "0"),
+ /**
+ * Copied from {@link GenericObjectPoolConfig#DEFAULT_MAX_IDLE} in Commons-DBCP 2.7.0
+ */
+ MAX_IDLE("8", "8"),
+ /**
+ * Copied from private variable {@link BasicDataSource#maxConnLifetimeMillis} in Commons-DBCP 2.7.0
+ */
+ MAX_CONN_LIFETIME("-1", "-1"),
+ /**
+ * Copied from {@link GenericObjectPoolConfig#DEFAULT_TIME_BETWEEN_EVICTION_RUNS_MILLIS} in Commons-DBCP 2.7.0
+ */
+ EVICTION_RUN_PERIOD("-1", "-1"),
+ /**
+ * Copied from {@link GenericObjectPoolConfig#DEFAULT_MIN_EVICTABLE_IDLE_TIME_MILLIS} in Commons-DBCP 2.7.0
+ * and converted from 1800000L to "1800000 millis" to "30 mins"
+ */
+ MIN_EVICTABLE_IDLE_TIME_MILLIS("1800000", "30 mins"),
Review Comment:
```suggestion
MIN_EVICTABLE_IDLE_TIME("1800000", "30 mins"),
```
Other constants (`MAX_CONN_LIFETIME`, `EVICTION_RUN_PERIOD`) do not have `_MILLIS` suffix which I think is correct because the time period can be defined in different time units.
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/utils/DataSourceConfiguration.java:
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.dbcp.utils;
+
+public class DataSourceConfiguration {
+
+ private final long maxWaitMillis;
+ private final int maxTotal;
+ private final int minIdle;
+ private final int maxIdle;
+ private final long maxConnLifetimeMillis;
+ private final long timeBetweenEvictionRunsMillis;
+ private final long minEvictableIdleTimeMillis;
+ private final long softMinEvictableIdleTimeMillis;
+ private final String validationQuery;
+ private final String url;
+ private final String driverName;
+ private final String userName;
+ private final String password;
+
+ public long getMaxWaitMillis() {
+ return maxWaitMillis;
+ }
+
+ public int getMaxTotal() {
+ return maxTotal;
+ }
+
+ public int getMinIdle() {
+ return minIdle;
+ }
+
+ public int getMaxIdle() {
+ return maxIdle;
+ }
+
+ public long getMaxConnLifetimeMillis() {
+ return maxConnLifetimeMillis;
+ }
+
+ public long getTimeBetweenEvictionRunsMillis() {
+ return timeBetweenEvictionRunsMillis;
+ }
+
+ public long getMinEvictableIdleTimeMillis() {
+ return minEvictableIdleTimeMillis;
+ }
+
+ public long getSoftMinEvictableIdleTimeMillis() {
+ return softMinEvictableIdleTimeMillis;
+ }
+
+ public String getValidationQuery() {
+ return validationQuery;
+ }
+
+ public String getUrl() {
+ return url;
+ }
+
+ public String getDriverName() {
+ return driverName;
+ }
+
+ public String getUserName() {
+ return userName;
+ }
+
+ public String getPassword() {
+ return password;
+ }
+
+ public DataSourceConfiguration(final Builder builder) {
+ this.maxWaitMillis = builder.maxWaitMillis;
+ this.maxTotal = builder.maxTotal;
+ this.minIdle = builder.minIdle;
+ this.maxIdle = builder.maxIdle;
+ this.maxConnLifetimeMillis = builder.maxConnLifetimeMillis;
+ this.timeBetweenEvictionRunsMillis = builder.timeBetweenEvictionRunsMillis;
+ this.minEvictableIdleTimeMillis = builder.minEvictableIdleTimeMillis;
+ this.softMinEvictableIdleTimeMillis = builder.softMinEvictableIdleTimeMillis;
+ this.validationQuery = builder.validationQuery;
+ this.url = builder.url;
+ this.driverName = builder.driverName;
+ this.userName = builder.userName;
+ this.password = builder.password;
+ }
+
+ public static class Builder {
+ private long maxWaitMillis = DefaultDataSourceValues.MAX_WAIT_MILLIS.getLongValue();
+ private int maxTotal = DefaultDataSourceValues.MAX_TOTAL_CONNECTIONS.getIntValue();
+ private int minIdle = DefaultDataSourceValues.MIN_IDLE.getIntValue();
+ private int maxIdle = DefaultDataSourceValues.MAX_IDLE.getIntValue();
+ private long maxConnLifetimeMillis = DefaultDataSourceValues.MAX_CONN_LIFETIME.getLongValue();
+ private long timeBetweenEvictionRunsMillis = DefaultDataSourceValues.EVICTION_RUN_PERIOD.getLongValue();
+ private long minEvictableIdleTimeMillis = DefaultDataSourceValues.MIN_EVICTABLE_IDLE_TIME_MILLIS.getLongValue();
+ private long softMinEvictableIdleTimeMillis = DefaultDataSourceValues.SOFT_MIN_EVICTABLE_IDLE_TIME.getLongValue();
+ private String validationQuery;
+ private final String url;
+ private final String driverName;
+ private final String userName;
+ private final String password;
Review Comment:
Minor, but I would move the more important and mandatory fields to the top (all over the class: fields, getters, etc.). Something like:
```
private final String url;
private final String driverName;
private final String userName;
private final String password;
private String validationQuery;
// other
```
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/AbstractDBCPConnectionPool.java:
##########
@@ -365,168 +121,151 @@ public List<ConfigVerificationResult> verify(final ConfigurationContext context,
verificationLogger.error("Failed to shut down data source", e);
}
}
-
return results;
}
/**
* Configures connection pool by creating an instance of the
* {@link BasicDataSource} based on configuration provided with
* {@link ConfigurationContext}.
- *
+ * <p>
* This operation makes no guarantees that the actual connection could be
* made since the underlying system may still go off-line during normal
* operation of the connection pool.
*
- * @param context
- * the configuration context
- * @throws InitializationException
- * if unable to create a database connection
+ * @param context the configuration context
+ * @throws InitializationException if unable to create a database connection
*/
@OnEnabled
public void onConfigured(final ConfigurationContext context) throws InitializationException {
- kerberosUser = getKerberosUser(context);
dataSource = new BasicDataSource();
- configureDataSource(dataSource, kerberosUser, context);
+ kerberosUser = getKerberosUser(context);
+ loginKerberos(kerberosUser);
+ final DataSourceConfiguration configuration = createDataSourceConfiguraton(context);
+ configureDataSource(context, configuration);
}
- private void configureDataSource(final BasicDataSource dataSource, final KerberosUser kerberosUser,
- final ConfigurationContext context) throws InitializationException {
- final String dburl = getUrl(context);
-
- final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
- final String user = context.getProperty(DB_USER).evaluateAttributeExpressions().getValue();
- final String passw = 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());
-
+ private void loginKerberos(KerberosUser kerberosUser) throws InitializationException {
if (kerberosUser != null) {
try {
kerberosUser.login();
} catch (KerberosLoginException e) {
throw new InitializationException("Unable to authenticate Kerberos principal", e);
}
}
+ }
- dataSource.setDriver(getDriver(driverName, dburl));
- dataSource.setMaxWaitMillis(maxWaitMillis);
- dataSource.setMaxTotal(maxTotal);
- dataSource.setMinIdle(minIdle);
- dataSource.setMaxIdle(maxIdle);
- dataSource.setMaxConnLifetimeMillis(maxConnLifetimeMillis);
- dataSource.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
- dataSource.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
- dataSource.setSoftMinEvictableIdleTimeMillis(softMinEvictableIdleTimeMillis);
+ protected abstract Driver getDriver(final String driverName, final String url);
- if (validationQuery != null && !validationQuery.isEmpty()) {
+ protected DataSourceConfiguration createDataSourceConfiguraton(final ConfigurationContext context) {
+ final String url = context.getProperty(DBCPProperties.DATABASE_URL).getValue();
+ final String user = context.getProperty(DBCPProperties.DB_USER).getValue();
+ final String password = context.getProperty(DBCPProperties.DB_PASSWORD).getValue();
+ final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
+
+ return new DataSourceConfiguration.Builder(url, driverName, user, password)
+ .build();
+ }
Review Comment:
This method it not really reusable unless the child processor has this 4 properties and no more because the configuration object is built right here and now.
I think the Builder should be returned instead in order to make the building of the configuration object extendable.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] Lehel44 commented on a diff in pull request #6935: NIFI-11151: Improving code reusability of DBCP services
Posted by "Lehel44 (via GitHub)" <gi...@apache.org>.
Lehel44 commented on code in PR #6935:
URL: https://github.com/apache/nifi/pull/6935#discussion_r1112255835
##########
nifi-nar-bundles/nifi-extension-utils/nifi-dbcp-base/src/main/java/org/apache/nifi/dbcp/AbstractDBCPConnectionPool.java:
##########
@@ -365,168 +121,151 @@ public List<ConfigVerificationResult> verify(final ConfigurationContext context,
verificationLogger.error("Failed to shut down data source", e);
}
}
-
return results;
}
/**
* Configures connection pool by creating an instance of the
* {@link BasicDataSource} based on configuration provided with
* {@link ConfigurationContext}.
- *
+ * <p>
* This operation makes no guarantees that the actual connection could be
* made since the underlying system may still go off-line during normal
* operation of the connection pool.
*
- * @param context
- * the configuration context
- * @throws InitializationException
- * if unable to create a database connection
+ * @param context the configuration context
+ * @throws InitializationException if unable to create a database connection
*/
@OnEnabled
public void onConfigured(final ConfigurationContext context) throws InitializationException {
- kerberosUser = getKerberosUser(context);
dataSource = new BasicDataSource();
- configureDataSource(dataSource, kerberosUser, context);
+ kerberosUser = getKerberosUser(context);
+ loginKerberos(kerberosUser);
+ final DataSourceConfiguration configuration = createDataSourceConfiguraton(context);
+ configureDataSource(context, configuration);
}
- private void configureDataSource(final BasicDataSource dataSource, final KerberosUser kerberosUser,
- final ConfigurationContext context) throws InitializationException {
- final String dburl = getUrl(context);
-
- final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
- final String user = context.getProperty(DB_USER).evaluateAttributeExpressions().getValue();
- final String passw = 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());
-
+ private void loginKerberos(KerberosUser kerberosUser) throws InitializationException {
if (kerberosUser != null) {
try {
kerberosUser.login();
} catch (KerberosLoginException e) {
throw new InitializationException("Unable to authenticate Kerberos principal", e);
}
}
+ }
- dataSource.setDriver(getDriver(driverName, dburl));
- dataSource.setMaxWaitMillis(maxWaitMillis);
- dataSource.setMaxTotal(maxTotal);
- dataSource.setMinIdle(minIdle);
- dataSource.setMaxIdle(maxIdle);
- dataSource.setMaxConnLifetimeMillis(maxConnLifetimeMillis);
- dataSource.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
- dataSource.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
- dataSource.setSoftMinEvictableIdleTimeMillis(softMinEvictableIdleTimeMillis);
+ protected abstract Driver getDriver(final String driverName, final String url);
- if (validationQuery != null && !validationQuery.isEmpty()) {
+ protected DataSourceConfiguration createDataSourceConfiguraton(final ConfigurationContext context) {
+ final String url = context.getProperty(DBCPProperties.DATABASE_URL).getValue();
+ final String user = context.getProperty(DBCPProperties.DB_USER).getValue();
+ final String password = context.getProperty(DBCPProperties.DB_PASSWORD).getValue();
+ final String driverName = context.getProperty(DB_DRIVERNAME).evaluateAttributeExpressions().getValue();
+
+ return new DataSourceConfiguration.Builder(url, driverName, user, password)
+ .build();
+ }
Review Comment:
These four properties are the necessary ones and the optionals are set to default values. I'd assume those won't be modified very often. What do you think about adding a copy constructor to DataSourceConfiguration, so the extended classes could do something like `new DataSourceConfiguration(extendedConfig).Builder().optionalConfigValue1(12)...build()` ?
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] Lehel44 commented on pull request #6935: NIFI-11151: Improving code reusability of DBCP services
Posted by "Lehel44 (via GitHub)" <gi...@apache.org>.
Lehel44 commented on PR #6935:
URL: https://github.com/apache/nifi/pull/6935#issuecomment-1437631254
Thanks for the comments @turcsanyip. I tested the DBCPConnectionPool manually and it worked fine.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org