tpalfy commented on pull request #5692:
URL: https://github.com/apache/nifi/pull/5692#issuecomment-1032869488
Let me try to explain in more detail my concerns through a demonstration.
Here's how I would imagine the propertyDescriptor-related duplication
removal:
1. We would need to settle for the properties not to have different `name`
and `displayName`. Btw this is a decision that is basically final. We won't be
able back down on that.
2. Here's how the code would look like:
Instead of this:
```java
private static final List<PropertyDescriptor> properties;
static {
final List<PropertyDescriptor> props = new ArrayList<>();
props.add(SNOWFLAKE_URL);
props.add(SNOWFLAKE_USER);
props.add(SNOWFLAKE_PASSWORD);
props.add(VALIDATION_QUERY);
props.add(MAX_WAIT_TIME);
props.add(MAX_TOTAL_CONNECTIONS);
props.add(MIN_IDLE);
props.add(MAX_IDLE);
props.add(MAX_CONN_LIFETIME);
props.add(EVICTION_RUN_PERIOD);
props.add(MIN_EVICTABLE_IDLE_TIME);
props.add(SOFT_MIN_EVICTABLE_IDLE_TIME);
properties = Collections.unmodifiableList(props);
}
@Override
protected List<PropertyDescriptor> getSupportedPropertyDescriptors() {
return properties;
}
```
We would have this:
```java
@Override
protected List<PropertyDescriptor> getSupportedPropertyDescriptors() {
final List<PropertyDescriptor> props = new ArrayList<>();
Collection<PropertyDescriptor>
becauseImSnowflakeIDontNeedTheseProperties = Arrays.asList(
DATABASE_URL,
DB_DRIVERNAME,
DB_DRIVER_LOCATION,
KERBEROS_USER_SERVICE,
KERBEROS_CREDENTIALS_SERVICE,
KERBEROS_PRINCIPAL,
KERBEROS_PASSWORD,
DB_USER,
DB_PASSWORD,
VALIDATION_QUERY
);
props.add(SNOWFLAKE_URL);
props.add(SNOWFLAKE_USER);
props.add(SNOWFLAKE_PASSWORD);
props.add(VALIDATION_QUERY);
props.addAll(super.getSupportedPropertyDescriptors());
props.removeAll(becauseImSnowflakeIDontNeedTheseProperties);
return props;
}
```
To me there's no question that the former is better even if we agree to 1.
With the datasource duplication we would do something like this:
In DBCPConnectionPool we would add the following method:
```java
protected void setDatasourceSettingsExceptDriverOfCourse(
String dburl,
String user,
String passw,
Integer maxTotal,
Long maxWaitMillis,
Integer minIdle,
Integer maxIdle,
Long maxConnLifetimeMillis,
Long timeBetweenEvictionRunsMillis,
Long minEvictableIdleTimeMillis,
Long softMinEvictableIdleTimeMillis
) {
dataSource.setUrl(dburl);
dataSource.setUsername(user);
dataSource.setPassword(passw);
dataSource.setMaxWaitMillis(maxWaitMillis);
dataSource.setMaxTotal(maxTotal);
dataSource.setMinIdle(minIdle);
dataSource.setMaxIdle(maxIdle);
dataSource.setMaxConnLifetimeMillis(maxConnLifetimeMillis);
dataSource.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
dataSource.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
dataSource.setSoftMinEvictableIdleTimeMillis(softMinEvictableIdleTimeMillis);
}
```
And instead of this:
```java
dataSource = new BasicDataSource();
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);
if (validationQuery!=null && !validationQuery.isEmpty()) {
dataSource.setValidationQuery(validationQuery);
dataSource.setTestOnBorrow(true);
}
dataSource.setUrl(dburl);
dataSource.setUsername(user);
dataSource.setPassword(passw);
```
We would have this:
```java
dataSource = new BasicDataSource();
dataSource.setDriver(getDriver(driverName, dburl));
setDatasourceSettingsExceptDriverOfCourse(
dburl,
user,
passw,
maxTotal,
maxWaitMillis,
minIdle,
maxIdle,
maxConnLifetimeMillis,
timeBetweenEvictionRunsMillis,
minEvictableIdleTimeMillis,
softMinEvictableIdleTimeMillis
);
```
And in SnowflakeComputingConnectionPool instead of this:
```java
dataSource.setUrl(connectionString);
dataSource.setUsername(user);
dataSource.setPassword(password);
dataSource.setMaxWaitMillis(maxWaitMillis);
dataSource.setMaxTotal(maxTotal);
dataSource.setMinIdle(minIdle);
dataSource.setMaxIdle(maxIdle);
dataSource.setMaxConnLifetimeMillis(maxConnLifetimeMillis);
dataSource.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
dataSource.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
dataSource.setSoftMinEvictableIdleTimeMillis(softMinEvictableIdleTimeMillis);
```
We would have this:
```java
setDatasourceSettingsExceptDriverOfCourse(
connectionString,
user,
password,
maxTotal,
maxWaitMillis,
minIdle,
maxIdle,
maxConnLifetimeMillis,
timeBetweenEvictionRunsMillis,
minEvictableIdleTimeMillis,
softMinEvictableIdleTimeMillis
);
```
To me this doesn't feel an improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
