[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
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...
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 issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
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...
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...
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 issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
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 issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
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...
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...
Github user colindean commented on the issue: https://github.com/apache/nifi/pull/3133 Test failure is on the Japanese build. ---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
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...
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...
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. ---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
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...
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...
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 3 milliseconds at org.apache.nifi.processors.standard.TestHandleHttpRequest.testMultipartFormDataRequest(TestHandleHttpRequest.java:271) ``` ---
[GitHub] nifi issue #3133: NIFI-5790: Exposes 6 commons-dbcp options in DBCPConnectio...
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...
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...
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⦠---
