You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by mosermw <gi...@git.apache.org> on 2016/11/09 18:38:03 UTC
[GitHub] nifi pull request #1192: NIFI-2996 validate processors only when they are in...
GitHub user mosermw opened a pull request:
https://github.com/apache/nifi/pull/1192
NIFI-2996 validate processors only when they are in STOPPED state
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.
- [ ] 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:
- [ ] 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?
- [ ] 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?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mosermw/nifi nifi-2996
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/1192.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 #1192
----
commit 73d444c8164387ea7ed8cc1bc6f118d566f87ed7
Author: Mike Moser <mo...@apache.org>
Date: 2016-11-09T17:38:04Z
NIFI-2996 validate processors only when they are in STOPPED state
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1192: NIFI-2996 validate processors only when they are in STOPPE...
Posted by mosermw <gi...@git.apache.org>.
Github user mosermw commented on the issue:
https://github.com/apache/nifi/pull/1192
Very good points @mcgilman. Just by numbers, there are more processors than services/tasks/ports, but I agree they should all act the same. It's possible that only the getValidationErrors() has significant effect on REST request performance. I will do more experiments and update this PR.
I really wanted this PR to have minimal changes, because I think NIFI-950 (background asynchronous validation when validating **everything** for the UI) is a better approach.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1192: NIFI-2996 validate processors only when they are in STOPPE...
Posted by mosermw <gi...@git.apache.org>.
Github user mosermw commented on the issue:
https://github.com/apache/nifi/pull/1192
@mcgilman I updated this PR for the following:
- reverted isValid() change
- modified FlowController to avoid calling isValid() if disabled or running. this matches similar logic for ports that is already in place
- modified getValidationErrors() to be all or nothing
- make getValidationErrors() act the same for Processors, Ports, Reporting Tasks, and Controller Services
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1192: NIFI-2996 validate processors only when they are in STOPPE...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/1192
Reviewing...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1192: NIFI-2996 validate processors only when they are in STOPPE...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/1192
Thanks @mosermw. This has been merged to master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1192: NIFI-2996 validate processors only when they are in...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/1192
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1192: NIFI-2996 validate processors only when they are in STOPPE...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/1192
@mosermw I reviewed the code and all looks good. I test with GetFile. Ensured that it was invalid when pointing to a directory that didn't exist. Created directory and refreshed status and saw that it was now valid. Start the processor, verified that it worked. Then deleted the directory and saw bulletins appear. Stopped Processor and saw the processor now shows as invalid, as expected. I'm a +1 but will hold off on merging until @mcgilman has a chance to review and/or comment, since he performed the original review. Thanks for digging in here!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1192: NIFI-2996 validate processors only when they are in STOPPE...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/1192
Thanks for the PR @mosermw! I was just running it and had some follow-up questions. It looks as though the proposed changes are conditionally validating the underlying Processor when it is stopped. However, other validation about the Processor (specifically regarding the incoming and outgoing connections) is still happening. Comments about these changes are outlined below according to the modified method.
**isValid**
isValid is used throughout the application to check processor state usually prior to performing some action or transitioning to some state. I believe in this method we still need to run all Processor validation logic to ensure we know if it's valid when invoking it.
When looking into the context of the isValid usage, it does appear that we can make some changes when getting the Processor status [1]. By checking if the scheduled state is running prior to checking the processor validity aligns with the proposed solution and should help reduce the expense of the status calls identified in the JIRA.
**getValidationErrors**
Regardless of whether the UI is rendering the validation results, they are still being returned from the Rest Api. I'm wondering if returning a portion of the validation errors when the Processor is not stopped is confusing. Would it be more clear if we moved the rest of the validation inside of the conditional as well? Meaning, when the Processor is not stopped, this method would return an empty Collection. When the Processor is stopped, this method would run all Processor validation.
**Additional Comment**
Should these changes also be applied to Ports, Reporting Tasks, and Controller Services (if disabled)? I understand that not all of these components cause lengthy validation times, but I'm trying to consider it from a consistency perspective from a client of the Rest Api. If a Processor only returns validation errors when it is stopped, I believe this is how all components should work too.
[1] https://github.com/apache/nifi/blob/master/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/FlowController.java#L2704
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---