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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
