You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/07/28 20:48:58 UTC
[GitHub] [nifi] adenes opened a new pull request #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
adenes opened a new pull request #5259:
URL: https://github.com/apache/nifi/pull/5259
### 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 `main`)?
- [x] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### 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?
- [x] Have you written or updated unit tests to verify your changes?
- [x] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] 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`?
- [x] 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?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
--
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] adenes commented on pull request #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
Posted by GitBox <gi...@apache.org>.
adenes commented on pull request #5259:
URL: https://github.com/apache/nifi/pull/5259#issuecomment-892950171
> Thanks for making the adjustments and writing up the additional issues @adenes. The improvements here look good, I noted one additional adjustment to reuse the static default value. The change should be made in both files.
Good point @exceptionfactory , thanks, I have updated the PR.
--
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 #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #5259:
URL: https://github.com/apache/nifi/pull/5259
--
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 change in pull request #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5259:
URL: https://github.com/apache/nifi/pull/5259#discussion_r678718375
##########
File path: nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/pom.xml
##########
@@ -121,6 +121,11 @@
<artifactId>commons-text</artifactId>
<version>1.8</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-dbcp2</artifactId>
+ <version>2.7.0</version>
Review comment:
The latest version of Apache Commons DBCP2 appears to be 2.8.0, can this be updated? It appears that other components use version 2.7.0, so if this update works as expected, those other references could also be updated.
--
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] adenes commented on pull request #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
Posted by GitBox <gi...@apache.org>.
adenes commented on pull request #5259:
URL: https://github.com/apache/nifi/pull/5259#issuecomment-892111817
Thanks for the suggestion @exceptionfactory , I have update the PR accordingly.
I didn't touch the `DBCPConnectionPool` and `Hive3ConnectionPool` classes to not to extend the scope here, but created a follow-up jira: https://issues.apache.org/jira/browse/NIFI-8994
(And also created one to track the Commons DBCP2 upgrade you asked for earlier: https://issues.apache.org/jira/browse/NIFI-8995)
--
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] adenes commented on pull request #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
Posted by GitBox <gi...@apache.org>.
adenes commented on pull request #5259:
URL: https://github.com/apache/nifi/pull/5259#issuecomment-891788038
Thanks @exceptionfactory for the review.
Actually I have copied the `-1` vs `TimePeriod` logic (and the implementation as well) from the [`Hive3ConnectionPool`](https://github.com/apache/nifi/blob/main/nifi-nar-bundles/nifi-hive-bundle/nifi-hive3-processors/src/main/java/org/apache/nifi/dbcp/hive/Hive3ConnectionPool.java#L473-L475) to be consistent with it (and also with the [`DBCPConnectionPool`](https://github.com/apache/nifi/blob/main/nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java)).
I see 2 more approaches here:
1) the one you suggested, i.e. don't use `-1` but use `0` for the infinite connection lifetime (and get rid of the `extractMillisWithInfinite` method)
2) my other idea is to set this property to optional and use the `null` value for the infinite lifetime (and that'd be the default obviously).
None of these are consistent with the other implementation, but I'm not sure we should insist on it. If we decide not using `-1` then I'd go with approach 2.
What do you think?
--
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 change in pull request #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5259:
URL: https://github.com/apache/nifi/pull/5259#discussion_r681002615
##########
File path: nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/dbcp/hive/HiveConnectionPool.java
##########
@@ -426,4 +445,9 @@ public String getConnectionURL() {
boolean isAllowExplicitKeytab() {
return Boolean.parseBoolean(System.getenv(ALLOW_EXPLICIT_KEYTAB));
}
+
+ private Long extractMillisWithInfinite(PropertyValue prop) {
Review comment:
The return type of `Long` implies that the return can be `null`, but `BasicDataSource.setMaxConnLifetimeMillis()` takes a primitive `long`. It would be helpful to check for `null` from `asTimePeriod()` and handle the scenario, then change this return type to `long`.
```suggestion
private long extractMillisWithInfinite(final PropertyValue prop) {
```
##########
File path: nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/dbcp/hive/HiveConnectionPool.java
##########
@@ -426,4 +445,9 @@ public String getConnectionURL() {
boolean isAllowExplicitKeytab() {
return Boolean.parseBoolean(System.getenv(ALLOW_EXPLICIT_KEYTAB));
}
+
+ private Long extractMillisWithInfinite(PropertyValue prop) {
+ return "-1".equals(prop.getValue()) ? -1 : prop.asTimePeriod(TimeUnit.MILLISECONDS);
Review comment:
Rather than have special handling for `-1`, it seems better to delegate all conversion to `asTimePeriod()` and set the default value to `0 s`. What do you think?
--
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] adenes commented on pull request #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
Posted by GitBox <gi...@apache.org>.
adenes commented on pull request #5259:
URL: https://github.com/apache/nifi/pull/5259#issuecomment-892950171
> Thanks for making the adjustments and writing up the additional issues @adenes. The improvements here look good, I noted one additional adjustment to reuse the static default value. The change should be made in both files.
Good point @exceptionfactory , thanks, I have updated the PR.
--
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 pull request #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #5259:
URL: https://github.com/apache/nifi/pull/5259#issuecomment-891814546
> Thanks @exceptionfactory for the review.
> Actually I have copied the `-1` vs `TimePeriod` logic (and the implementation as well) from the [`Hive3ConnectionPool`](https://github.com/apache/nifi/blob/main/nifi-nar-bundles/nifi-hive-bundle/nifi-hive3-processors/src/main/java/org/apache/nifi/dbcp/hive/Hive3ConnectionPool.java#L473-L475) to be consistent with it (and also with the [`DBCPConnectionPool`](https://github.com/apache/nifi/blob/main/nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/main/java/org/apache/nifi/dbcp/DBCPConnectionPool.java)).
>
> I see 2 more approaches here:
>
> 1. the one you suggested, i.e. don't use `-1` but use `0` for the infinite connection lifetime (and get rid of the `extractMillisWithInfinite` method)
>
> 2. my other idea is to set this property to optional and use the `null` value for the infinite lifetime (and that'd be the default obviously).
> None of these are consistent with the other implementation, but I'm not sure we should insist on it. If we decide not using `-1` then I'd go with approach 2.
>
>
> What do you think?
Thanks for pointing out the source of that approach @adenes. In that case, it seems reasonable to maintain the current implementation with support for `-1`. It appears that the methods in `DBCPConnectionPool` and `Hive3ConnectionPool` also have the potential issue with returning `null` from `PropertyValue.asTimePeriod()`.
With those options, I would check the return of `PropertyValue.asTimePeriod()` for `null` and return `-1` in that situation, to avoid the edge case of a null return, and adjust the return type of the extract method to `long` instead of `Long`.
--
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 change in pull request #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5259:
URL: https://github.com/apache/nifi/pull/5259#discussion_r682090858
##########
File path: nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/dbcp/hive/HiveConnectionPool.java
##########
@@ -426,4 +445,13 @@ public String getConnectionURL() {
boolean isAllowExplicitKeytab() {
return Boolean.parseBoolean(System.getenv(ALLOW_EXPLICIT_KEYTAB));
}
+
+ private long extractMillisWithInfinite(PropertyValue prop) {
+ if (prop.getValue() == null || "-1".equals(prop.getValue())) {
Review comment:
Minor adjustment to reuse the static default:
```suggestion
if (prop.getValue() == null || DEFAULT_MAX_CONN_LIFETIME.equals(prop.getValue())) {
```
--
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 pull request #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
Posted by GitBox <gi...@apache.org>.
turcsanyip commented on pull request #5259:
URL: https://github.com/apache/nifi/pull/5259#issuecomment-895212817
@adenes Thanks for adding the Max Connection Lifetime property!
@exceptionfactory Thanks for the review!
Merging to main...
--
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] adenes commented on a change in pull request #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
Posted by GitBox <gi...@apache.org>.
adenes commented on a change in pull request #5259:
URL: https://github.com/apache/nifi/pull/5259#discussion_r679471640
##########
File path: nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/pom.xml
##########
@@ -121,6 +121,11 @@
<artifactId>commons-text</artifactId>
<version>1.8</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-dbcp2</artifactId>
+ <version>2.7.0</version>
Review comment:
Thanks for the comment @exceptionfactory . I'd go with 2.7.0 for this change to touch as few places as possible and create a follow-up ticket to upgrade Apache Commons DBCP2 NiFi-wide. What do you think?
--
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 change in pull request #5259: NIFI-8955 Add Max Connection Lifetime property to Hive(_1_1)ConnectionPool CS
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5259:
URL: https://github.com/apache/nifi/pull/5259#discussion_r679493124
##########
File path: nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/pom.xml
##########
@@ -121,6 +121,11 @@
<artifactId>commons-text</artifactId>
<version>1.8</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-dbcp2</artifactId>
+ <version>2.7.0</version>
Review comment:
Thanks for following up @adenes, it might be easier to make the changes for these particular components in this pull request, but updating all references in a separate PR also works.
--
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