mattyb149 commented on a change in pull request #4588:
URL: https://github.com/apache/nifi/pull/4588#discussion_r524529511
##########
File path:
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/dbcp/hive/HiveConnectionPool.java
##########
@@ -348,6 +365,7 @@ public void onConfigured(final ConfigurationContext
context) throws Initializati
if (validationQuery != null && !validationQuery.isEmpty()) {
dataSource.setValidationQuery(validationQuery);
+ dataSource.setValidationQueryTimeout(validationQueryTimeout); //
timeout in seconds
Review comment:
`DBCPConnectionPool` has a `getDataSource()` method in order to
facilitate testing with mocks/stubs/delegates. It would be nice to have a unit
test there and here (if possible) to exercise the timeout logic. Perhaps we can
create a BasicDataSource delegate that returns mocked connections, and when a
certain validation query is called, we sleep for a time longer than the
timeout. I'm not exactly sure what that would look like, just tossing out an
idea :) Have you tested this on a real system and verified the timeout occurs
in such a situation?
##########
File path:
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/dbcp/hive/HiveConnectionPool.java
##########
@@ -71,6 +72,9 @@
@RequiresInstanceClassLoading
@Tags({"hive", "dbcp", "jdbc", "database", "connection", "pooling", "store"})
@CapabilityDescription("Provides Database Connection Pooling Service for
Apache Hive. Connections can be asked from pool and returned after usage.")
+@DynamicProperty(name = "The name of a Hive configuration property.", value =
"The value of the given Hive configuration property.",
Review comment:
I definitely appreciate your effort to add documentation that was
missing from already-implemented features. However as it turns out, dynamic
properties don't actually work for the Hive connection pools, so I wrote up
[NIFI-8016](https://issues.apache.org/jira/browse/NIFI-8016) to cover this (and
actually to change the behavior). Because I expect a behavior change, the
documentation should be added as part of that Jira to explain what it does once
correctly implemented.
Do you mind removing the doc from this PR? I can add this as part of
NIFI-8016 (or you can if you like 😄 )
##########
File path:
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/dbcp/hive/HiveConnectionPool.java
##########
@@ -150,6 +154,17 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor VALIDATION_QUERY_TIMEOUT = new
PropertyDescriptor.Builder()
+ .name("Validation-query-timeout")
+ .displayName("Validation Query Timeout")
+ .description("The maximum amount of time allowed for running a
validation query, "
+ + "zero means there is no limit. Max time less than 1
second will be equal to zero.")
Review comment:
In general the DataSource method is expecting an integer number of
seconds right? So instead of "max time < 1 = 0" I think it's more accurate to
say the time will be converted (and truncated) to an integer number of seconds.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
