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/02/02 19:24:13 UTC
[GitHub] [nifi] JonathanKessler opened a new pull request #4800: NIFI-8180 Added a default value for connections' FlowFile Expiration …
JonathanKessler opened a new pull request #4800:
URL: https://github.com/apache/nifi/pull/4800
…Period to nifi.properties
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
Updates to the framework to pull a default value for a new Connection's FlowFile Expiration Period from nifi.properties.
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 `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? N/A
- [X] Have you verified that the full build is successful on JDK 8?
- [X] Have you verified that the full build is successful on JDK 11?
- [X] 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)? N/A
- [X] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`? N/A
- [X] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`? N/A
- [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:
- [X] 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] JonathanKessler commented on pull request #4800: NIFI-8180 Added a default value for connections' FlowFile Expiration …
Posted by GitBox <gi...@apache.org>.
JonathanKessler commented on pull request #4800:
URL: https://github.com/apache/nifi/pull/4800#issuecomment-772776718
@markap14 That's understandable. Not a problem, I appreciate your consideration.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4800: NIFI-8180 Added a default value for connections' FlowFile Expiration …
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4800:
URL: https://github.com/apache/nifi/pull/4800#issuecomment-772750238
I think even at a process group level, it is too dangerous. I don't think we should ever expire data on any connection without a user explicitly configuring that connection to do so.
There is a small indicator on the connection, but it is easily missed, especially if you're not looking for it. And many users, especially newer ones, may not know what that icon means. If they notice it and go searching to find what it means, it will be easy to figure out, but it won't necessarily jump out at them.
I would also consider it an anti-pattern to go through a flow and start marking all connections with expiration dates. Typically you have a flow that has many processors, and expiration would be configured on the end of that dataflow only. There's no real need to age the data off in the middle. If age off is configured only at the end, the data will age off as necessary, and even if there's backpressure applied the aging off will allow data to continue through the flow and get to the end where it will be aged off.
Certainly, configuring backpressure throughout the flow could result in being more efficient if you have a lot of data by aging off the old data more aggressively. But at what cost? If ageoff is set to 5 mins and you decide you now want it to be 10 minutes, you now have to go through potentially hundreds of connections and update them. This would get unwieldy very quickly.
I very much appreciate the work that you've put into this. It's a non-trivial PR, for sure. But given the likelihood of unintended data loss and the concerns about usability that it would introduce I'm a -1 on this feature.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] JonathanKessler closed pull request #4800: NIFI-8180 Added a default value for connections' FlowFile Expiration …
Posted by GitBox <gi...@apache.org>.
JonathanKessler closed pull request #4800:
URL: https://github.com/apache/nifi/pull/4800
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #4800: NIFI-8180 Added a default value for connections' FlowFile Expiration …
Posted by GitBox <gi...@apache.org>.
markobean commented on pull request #4800:
URL: https://github.com/apache/nifi/pull/4800#issuecomment-772735028
Also, note that this setting is simply a default for when a NEW connection is created. It does not affect settings of existing connections. Further, on the UI, the dataflow developer can see that there is an expiration set by looking at the connection, and can confirm its setting with a simple mouse-over.
I agree with @JonathanKessler to move this functionality to the Process Group level. It gives finer granularity of control as well as more flexible implementation (not requiring a restart to change settings.)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] JonathanKessler commented on pull request #4800: NIFI-8180 Added a default value for connections' FlowFile Expiration …
Posted by GitBox <gi...@apache.org>.
JonathanKessler commented on pull request #4800:
URL: https://github.com/apache/nifi/pull/4800#issuecomment-772720409
@markap14 You make a valid point. What are your thoughts on implementing this feature at a Process Group level instead? Though your concern would of course still be valid, it would give users more visibility that such a default was set and would also better support the notion of multi-tenancy. If a user were inclined to have a system-wide default, they could then set the value at the root level process group and all its descendants would inherit that default unless they were specifically overwritten.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4800: NIFI-8180 Added a default value for connections' FlowFile Expiration …
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4800:
URL: https://github.com/apache/nifi/pull/4800#issuecomment-772703211
Thanks for the contribution @JonathanKessler. I'm not sure this is a feature that we want to support, though. If someone were to change the value of that in nifi.properties, dataflow developers may well forget to change that, and it would almost certainly lead to unintended data loss.
----------------------------------------------------------------
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:
users@infra.apache.org