You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by colindean <gi...@git.apache.org> on 2018/11/05 21:24:59 UTC
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
GitHub user colindean opened a pull request:
https://github.com/apache/nifi/pull/3133
NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectionPool
These six options support the eviction and passivation of idle
connections.
This adds the feature described in NIFI-5790 in pursuit of a
fix for NIFI-5789.
----
Thank you for submitting a contribution to Apache NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [X] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [X] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [X] Is your initial contribution a single, squashed commit?
### For code changes:
- [X] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] ~~If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?~~
- [ ] ~~If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?~~
- [ ] ~~If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?~~
- [ ] ~~If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?~~
### For documentation related changes:
- [ ] ~~Have you ensured that format looks appropriate for the output in which it is rendered?~~
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/colindean/nifi nifi-5790_expose-dbcp-options
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/3133.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3133
----
commit 2bd51cbc7495529acc174655d187fdefb7451382
Author: Colin Dean <co...@...>
Date: 2018-11-05T21:18:21Z
NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectionPool
These six options support the eviction and passivation of idle
connections.
This adds the feature described in NIFI-5790 in pursuit of a
fix for NIFI-5789.
----
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r231654261
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -164,6 +161,71 @@ public ValidationResult validate(final String subject, final String input, final
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .name("Minimum Idle Connections")
+ .description("The minimum number of connections that can remain idle in the pool, without extra ones being " +
+ "created, or zero to create none.")
+ .defaultValue("0")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .sensitive(false)
+ .build();
+
+ public static final PropertyDescriptor MAX_IDLE = new PropertyDescriptor.Builder()
+ .name("Max Idle Connections")
+ .description("The maximum number of connections that can remain idle in the pool, without extra ones being " +
+ "released, or negative for no limit.")
+ .defaultValue("8")
--- End diff --
@mattyb149 Yes, it's 8. Should I reach into the impl to refer to the default constants?
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r230927408
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -164,6 +161,71 @@ public ValidationResult validate(final String subject, final String input, final
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .name("Minimum Idle Connections")
--- End diff --
I saw your comment about `.name`. I know the existing properties in DBCPConnectionPool do not use `displayName`, but that is only because they are from before the change in standards.
Can you move the new `name` property values to `displayName`, and set `name` to something like `dbcp-min-idle-conns`?
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on the issue:
https://github.com/apache/nifi/pull/3133
Test failure is on the Japanese build.
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r231660071
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -164,6 +164,71 @@ public ValidationResult validate(final String subject, final String input, final
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .displayName("Minimum Idle Connections")
+ .name("dbcp-mim-idle-conns")
+ .description("The minimum number of connections that can remain idle in the pool, without extra ones being " +
+ "created, or zero to create none.")
+ .defaultValue("0")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .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, or negative for no limit.")
+ .defaultValue("8")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .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("-1")
--- End diff --
Looking into it, `NONNEGATIVE_INTEGER_VALIDATOR` lacks the custom time period validation. The new time period options should continue to use the `CUSTOM_TIME_PERIOD_VALIDATOR` defined in this class but I'll change `MIN_IDLE` to use `NONNEGATIVE_INTEGER_VALIDATOR`. `MAX_IDLE` needs to stay `INTEGER_VALIDATOR` because 0 is a valid value for it.
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r230920291
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -164,6 +161,71 @@ public ValidationResult validate(final String subject, final String input, final
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .name("Minimum Idle Connections")
+ .description("The minimum number of connections that can remain idle in the pool, without extra ones being " +
+ "created, or zero to create none.")
+ .defaultValue("0")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .sensitive(false)
--- End diff --
You don't need `sensitive(false)`. I think you'r safe to remove it from these new properties.
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r231580368
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -164,6 +161,71 @@ public ValidationResult validate(final String subject, final String input, final
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .name("Minimum Idle Connections")
+ .description("The minimum number of connections that can remain idle in the pool, without extra ones being " +
+ "created, or zero to create none.")
+ .defaultValue("0")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .sensitive(false)
+ .build();
+
+ public static final PropertyDescriptor MAX_IDLE = new PropertyDescriptor.Builder()
+ .name("Max Idle Connections")
+ .description("The maximum number of connections that can remain idle in the pool, without extra ones being " +
+ "released, or negative for no limit.")
+ .defaultValue("8")
--- End diff --
IMO we should keep the default value as the one used when you don't explicitly set it, I guess that's 8?
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on the issue:
https://github.com/apache/nifi/pull/3133
@colindean Your correct, patch releases are for very special circumstances. I don't believe you have any option except to build this yourself until 1.9.0 gets released; and since 1.8.0 just came out, that is going to be a bit.
To keep things stable, you can checkout the 1.8.0 tag and apply your change as a cherry pick so you don't accidentally include any unexpected changes, but still an internal build.
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r230921657
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -164,6 +161,71 @@ public ValidationResult validate(final String subject, final String input, final
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .name("Minimum Idle Connections")
+ .description("The minimum number of connections that can remain idle in the pool, without extra ones being " +
+ "created, or zero to create none.")
+ .defaultValue("0")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .sensitive(false)
+ .build();
+
+ public static final PropertyDescriptor MAX_IDLE = new PropertyDescriptor.Builder()
+ .name("Max Idle Connections")
+ .description("The maximum number of connections that can remain idle in the pool, without extra ones being " +
+ "released, or negative for no limit.")
+ .defaultValue("8")
--- End diff --
Setting this to `8` feels so weird, even though it is legitimately the default value in the DBCP library.
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on the issue:
https://github.com/apache/nifi/pull/3133
Thank you for confirming. Looks like my team will have some work to do
next week!
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on the issue:
https://github.com/apache/nifi/pull/3133
Squash incoming…
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on the issue:
https://github.com/apache/nifi/pull/3133
@colindean I don't have a good answer yet, hoping I can get some input from other developers.
But I was thinking about unit tests, and what you could do to help make this code change unit testable.
What if you exposed the number of active and idle connections in the connection pool as properties on the DBCPConnectinoPool? These are available by calling `getNumActive()` and `getNumIdle()`. Or you could call `listAllObjects()` and get back the full pool on the `dataSource` object.
With these numbers it would be possible to test at least the min/max connection settings, and maybe more.
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on the issue:
https://github.com/apache/nifi/pull/3133
Very strange… it's there in the PR's commit and my local copy and Travis built fine:
https://github.com/apache/nifi/blob/a3868928e278dc6797367f97d1d84eebcf1aad38/nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java#L27
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r232011596
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -164,6 +166,77 @@ public ValidationResult validate(final String subject, final String input, final
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .displayName("Minimum Idle Connections")
+ .name("dbcp-mim-idle-conns")
+ .description("The minimum number of connections that can remain idle in the pool, without extra ones being " +
+ "created, or zero to create none.")
+ .defaultValue(String.valueOf(GenericObjectPoolConfig.DEFAULT_MIN_IDLE))
--- End diff --
I didn't realize the constants were only available from a class in an "impl" package in DBCP. In that case I take back what I said :( Can we create our own constants here with a comment that we got the values from DBCP itself?
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on the issue:
https://github.com/apache/nifi/pull/3133
> What if you exposed the number of active and idle connections in the connection pool as properties on the DBCPConnectionPool? These are available by calling getNumActive() and getNumIdle(). Or you could call listAllObjects() and get back the full pool on the dataSource object.
I started down this track but I couldn't actually seem to trigger idle within a reasonable test setup. I can't for the life of me tell what actually makes a connection go idle, only really what removes it _when_ it's idle. Thoughts on how to proceed?
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on the issue:
https://github.com/apache/nifi/pull/3133
> What about making all of the new fields not required? `required(false)`. Then, if a value is provided, you override, otherwise you leave it be. You could call out the default values, or default functionality at least, in the property description?
I _could_ go this route, but I'm not where to strike the balance between "explicit settings from defaults at the time of creation" versus "use the latest default". I could reference the DBCP default constants in the property description, too, thus preventing the description from becoming out of date…
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on the issue:
https://github.com/apache/nifi/pull/3133
I'll squash once travis says 🆗
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on the issue:
https://github.com/apache/nifi/pull/3133
@colindean I know Matt responded right after I did before, what are your thoughts on working on enabling unit tests by exposing the idle/active connection counts?
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on the issue:
https://github.com/apache/nifi/pull/3133
@colindean Disregard. Git was misbehaving.
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on the issue:
https://github.com/apache/nifi/pull/3133
Ah, I did that through checking the minIdle and maxIdle properties.
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on the issue:
https://github.com/apache/nifi/pull/3133
🎉
@patricker @mattyb149 How often and for what reasons does NiFi issue patch releases? Looking through the release history, it seems pretty rare and saved for regressions only. More directly, I'd love to get this into an official release sooner than later, in order to avoid the burdens of building NiFi internally or pulling in patched NARs as a part of our build process (we ship NiFi as a part of a product in development, but we're like _days_ from shipping 1.0…). I understand if there are rules/guidelines that would prevent this.
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on the issue:
https://github.com/apache/nifi/pull/3133
Test failure now seems to be a timeout in another module:
```
[ERROR] Tests run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 30.287 s <<< FAILURE! - in org.apache.nifi.processors.standard.TestHandleHttpRequest
[ERROR] testMultipartFormDataRequest(org.apache.nifi.processors.standard.TestHandleHttpRequest) Time elapsed: 30.012 s <<< ERROR!
org.junit.runners.model.TestTimedOutException: test timed out after 30000 milliseconds
at org.apache.nifi.processors.standard.TestHandleHttpRequest.testMultipartFormDataRequest(TestHandleHttpRequest.java:271)
```
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r231152202
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -164,6 +161,71 @@ public ValidationResult validate(final String subject, final String input, final
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .name("Minimum Idle Connections")
+ .description("The minimum number of connections that can remain idle in the pool, without extra ones being " +
+ "created, or zero to create none.")
+ .defaultValue("0")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .sensitive(false)
+ .build();
+
+ public static final PropertyDescriptor MAX_IDLE = new PropertyDescriptor.Builder()
+ .name("Max Idle Connections")
+ .description("The maximum number of connections that can remain idle in the pool, without extra ones being " +
+ "released, or negative for no limit.")
+ .defaultValue("8")
--- End diff --
@mattyb149 If you have a second, I'd appreciate your thoughts on this.
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r232317090
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -59,6 +61,31 @@
+ "Note that no flow file input (attributes, e.g.) is available for use in Expression Language constructs for these properties.")
public class DBCPConnectionPool extends AbstractControllerService implements DBCPService {
+ /**
+ * Copied from {@link GenericObjectPoolConfig.DEFAULT_MIN_IDLE} in Commons-DBCP 2.5.0
+ */
+ private static final String DEFAULT_MIN_IDLE = "0";
+ /**
+ * Copied from {@link GenericObjectPoolConfig.DEFAULT_MAX_IDLE} in Commons-DBCP 2.5.0
+ */
+ private static final String DEFAULT_MAX_IDLE = "8";
+ /**
+ * Copied from private variable {@link BasicDataSource.maxConnLifetimeMillis} in Commons-DBCP 2.5.0
+ */
+ private static final String DEFAULT_MAX_CONN_LIFETIME = "-1";
+ /**
+ * Copied from {@link GenericObjectPoolConfig.DEFAULT_TIME_BETWEEN_EVICTION_RUNS_MILLIS} in Commons-DBCP 2.5.0
+ */
+ private static final String DEFAULT_EVICTION_RUN_PERIOD = String.valueOf(-1L);
+ /**
+ * Copied from {@link GenericObjectPoolConfig.DEFAULT_MIN_EVICTABLE_IDLE_TIME_MILLIS} in Commons-DBCP 2.5.0
+ */
+ private static final String DEFAULT_MIN_EVICTABLE_IDLE_TIME = String.valueOf(1800000L) + " millis";
--- End diff --
Testing on my local instance is looking good so far. One request, can you change this to `30 min`? A bit easier to read as a default value that way :)
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on the issue:
https://github.com/apache/nifi/pull/3133
@colindean I am getting compile errors. I had to add an import, `import org.apache.nifi.components.PropertyValue;`, otherwise your method `extractMillisWithInfinite` would not build.
This is building OK for you? Maybe something got lost in the squash?
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r230935621
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -164,6 +161,71 @@ public ValidationResult validate(final String subject, final String input, final
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .name("Minimum Idle Connections")
+ .description("The minimum number of connections that can remain idle in the pool, without extra ones being " +
+ "created, or zero to create none.")
+ .defaultValue("0")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .sensitive(false)
+ .build();
+
+ public static final PropertyDescriptor MAX_IDLE = new PropertyDescriptor.Builder()
+ .name("Max Idle Connections")
+ .description("The maximum number of connections that can remain idle in the pool, without extra ones being " +
+ "released, or negative for no limit.")
+ .defaultValue("8")
--- End diff --
There are some defaults I could reference, e.g. `org.apache.commons.pool2.impl.BaseObjectPoolConfig#DEFAULT_MIN_EVICTABLE_IDLE_TIME_MILLIS` and `org.apache.commons.pool2.impl.GenericObjectPoolConfig#DEFAULT_MAX_IDLE`. Should I instead reach into DBCP to get those?
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r231582876
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -164,6 +164,71 @@ public ValidationResult validate(final String subject, final String input, final
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .displayName("Minimum Idle Connections")
+ .name("dbcp-mim-idle-conns")
+ .description("The minimum number of connections that can remain idle in the pool, without extra ones being " +
+ "created, or zero to create none.")
+ .defaultValue("0")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .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, or negative for no limit.")
+ .defaultValue("8")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .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("-1")
+ .required(true)
+ .addValidator(CUSTOM_TIME_PERIOD_VALIDATOR)
+ .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("-1")
+ .required(true)
+ .addValidator(CUSTOM_TIME_PERIOD_VALIDATOR)
+ .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("1800 secs")
+ .required(true)
+ .addValidator(CUSTOM_TIME_PERIOD_VALIDATOR)
--- End diff --
The validator supports expression language, so I think these properties should support expression language at the Variable Registry level
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r231580669
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -164,6 +164,71 @@ public ValidationResult validate(final String subject, final String input, final
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .displayName("Minimum Idle Connections")
+ .name("dbcp-mim-idle-conns")
+ .description("The minimum number of connections that can remain idle in the pool, without extra ones being " +
+ "created, or zero to create none.")
+ .defaultValue("0")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .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, or negative for no limit.")
+ .defaultValue("8")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .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("-1")
--- End diff --
Even if the API allows zero or less, we could just say zero means infinite and use a NONNEGATIVE_INTEGER_VALIDATOR
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r232065708
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/test/java/org/apache/nifi/dbcp/DBCPServiceTest.java ---
@@ -93,6 +94,155 @@ public void testMaxWait() throws InitializationException {
runner.assertValid(service);
}
+ /**
+ * Checks validity of idle limit and time settings including a default
+ */
+ @Test
+ public void testIdleConnectionsSettings() throws InitializationException {
+ final TestRunner runner = TestRunners.newTestRunner(TestProcessor.class);
+ final DBCPConnectionPool service = new DBCPConnectionPool();
+ runner.addControllerService("test-good1", service);
+
+ // remove previous test database, if any
+ final File dbLocation = new File(DB_LOCATION);
+ dbLocation.delete();
+
+ // set embedded Derby database connection url
+ runner.setProperty(service, DBCPConnectionPool.DATABASE_URL, "jdbc:derby:" + DB_LOCATION + ";create=true");
+ runner.setProperty(service, DBCPConnectionPool.DB_USER, "tester");
+ runner.setProperty(service, DBCPConnectionPool.DB_PASSWORD, "testerp");
+ runner.setProperty(service, DBCPConnectionPool.DB_DRIVERNAME, "org.apache.derby.jdbc.EmbeddedDriver");
+ runner.setProperty(service, DBCPConnectionPool.MAX_WAIT_TIME, "-1");
+ runner.setProperty(service, DBCPConnectionPool.MAX_IDLE, "2");
+ runner.setProperty(service, DBCPConnectionPool.MAX_CONN_LIFETIME, "1 secs");
+ runner.setProperty(service, DBCPConnectionPool.EVICTION_RUN_PERIOD, "1 secs");
+ runner.setProperty(service, DBCPConnectionPool.MIN_EVICTABLE_IDLE_TIME, "1 secs");
+ runner.setProperty(service, DBCPConnectionPool.SOFT_MIN_EVICTABLE_IDLE_TIME, "1 secs");
+
+ runner.enableControllerService(service);
+ runner.assertValid(service);
+ }
+
+ @Test
+ public void testMinIdleCannotBeNegative() throws InitializationException {
+ final TestRunner runner = TestRunners.newTestRunner(TestProcessor.class);
+ final DBCPConnectionPool service = new DBCPConnectionPool();
+ runner.addControllerService("test-good1", service);
+
+ // remove previous test database, if any
+ final File dbLocation = new File(DB_LOCATION);
+ dbLocation.delete();
+
+ // set embedded Derby database connection url
+ runner.setProperty(service, DBCPConnectionPool.DATABASE_URL, "jdbc:derby:" + DB_LOCATION + ";create=true");
+ runner.setProperty(service, DBCPConnectionPool.DB_USER, "tester");
+ runner.setProperty(service, DBCPConnectionPool.DB_PASSWORD, "testerp");
+ runner.setProperty(service, DBCPConnectionPool.DB_DRIVERNAME, "org.apache.derby.jdbc.EmbeddedDriver");
+ runner.setProperty(service, DBCPConnectionPool.MAX_WAIT_TIME, "-1");
+ runner.setProperty(service, DBCPConnectionPool.MIN_IDLE, "-1");
+
+ runner.assertNotValid(service);
+ }
+
+ /**
+ * Checks to ensure that settings have been passed down into the DBCP
+ */
+ @Test
+ public void testIdleSettingsAreSet() throws InitializationException, SQLException {
+ final TestRunner runner = TestRunners.newTestRunner(TestProcessor.class);
+ final DBCPConnectionPool service = new DBCPConnectionPool();
+ runner.addControllerService("test-good1", service);
+
+ // remove previous test database, if any
+ final File dbLocation = new File(DB_LOCATION);
+ dbLocation.delete();
+
+ // set embedded Derby database connection url
+ runner.setProperty(service, DBCPConnectionPool.DATABASE_URL, "jdbc:derby:" + DB_LOCATION + ";create=true");
+ runner.setProperty(service, DBCPConnectionPool.DB_USER, "tester");
+ runner.setProperty(service, DBCPConnectionPool.DB_PASSWORD, "testerp");
+ runner.setProperty(service, DBCPConnectionPool.DB_DRIVERNAME, "org.apache.derby.jdbc.EmbeddedDriver");
+ runner.setProperty(service, DBCPConnectionPool.MAX_WAIT_TIME, "-1");
+ runner.setProperty(service, DBCPConnectionPool.MAX_IDLE, "6");
+ runner.setProperty(service, DBCPConnectionPool.MIN_IDLE, "4");
+ runner.setProperty(service, DBCPConnectionPool.MAX_CONN_LIFETIME, "1 secs");
+ runner.setProperty(service, DBCPConnectionPool.EVICTION_RUN_PERIOD, "1 secs");
+ runner.setProperty(service, DBCPConnectionPool.MIN_EVICTABLE_IDLE_TIME, "1 secs");
+ runner.setProperty(service, DBCPConnectionPool.SOFT_MIN_EVICTABLE_IDLE_TIME, "1 secs");
+
+ runner.enableControllerService(service);
+
+ Assert.assertEquals(6, service.getDataSource().getMaxIdle());
+ Assert.assertEquals(4, service.getDataSource().getMinIdle());
+ Assert.assertEquals(1000, service.getDataSource().getMaxConnLifetimeMillis());
+ Assert.assertEquals(1000, service.getDataSource().getTimeBetweenEvictionRunsMillis());
+ Assert.assertEquals(1000, service.getDataSource().getMinEvictableIdleTimeMillis());
+ Assert.assertEquals(1000, service.getDataSource().getSoftMinEvictableIdleTimeMillis());
+
+ service.getDataSource().close();
+ }
+
+ /**
+ * Creates a few connections and step closes them to see what happens
+ */
+ @Test
+ public void testIdle() throws InitializationException, SQLException, InterruptedException {
--- End diff --
Exactly what I was hoping to see in the tests. Thanks for making these!
I think we are getting close. After @mattyb149 's requested change, probably a good time to squash your commit's.
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/3133
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on the issue:
https://github.com/apache/nifi/pull/3133
> If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
I added properties but I used a readable string for `.name`.
> Have you written or updated unit tests to verify your changes?
I'm not sure how best I can _operationally_ test these, or if that makes sense because we'd be testing the API functionality of commons-dbcp…
---
[GitHub] nifi pull request #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPCo...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3133#discussion_r231582349
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java ---
@@ -164,6 +164,71 @@ public ValidationResult validate(final String subject, final String input, final
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor MIN_IDLE = new PropertyDescriptor.Builder()
+ .displayName("Minimum Idle Connections")
+ .name("dbcp-mim-idle-conns")
+ .description("The minimum number of connections that can remain idle in the pool, without extra ones being " +
+ "created, or zero to create none.")
+ .defaultValue("0")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .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, or negative for no limit.")
+ .defaultValue("8")
+ .required(true)
+ .addValidator(StandardValidators.INTEGER_VALIDATOR)
+ .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("-1")
+ .required(true)
+ .addValidator(CUSTOM_TIME_PERIOD_VALIDATOR)
+ .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("-1")
--- End diff --
If the default values are available as constants from DBCP, we should probably use those. If we want to limit "zero or less" to just zero, we can just do an additional Math.max()
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by colindean <gi...@git.apache.org>.
Github user colindean commented on the issue:
https://github.com/apache/nifi/pull/3133
> an Idle connection was one that had been returned to the pool?
That's what I would think, but I couldn't seem to actually trigger it.
Reading through the API docs some more, I didn't think to try checking the idle connection count _after_ closing a connection. I'll try that tomorrow.
---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
Posted by patricker <gi...@git.apache.org>.
Github user patricker commented on the issue:
https://github.com/apache/nifi/pull/3133
My thought was you could look for the idle count and see if it was 0, 8, etc... based on the config, and not worry about testing the timeouts for now.
---