You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by mcgilman <gi...@git.apache.org> on 2018/05/18 18:59:12 UTC
[GitHub] nifi pull request #2722: NIFI-5186: Updating UI to support asynchronous vali...
GitHub user mcgilman opened a pull request:
https://github.com/apache/nifi/pull/2722
NIFI-5186: Updating UI to support asynchronous validation
NIFI-5186:
- Updating UI to support showing when a component is validating.
- Code clean up.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mcgilman/nifi NIFI-5186
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2722.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 #2722
----
commit b4d3a48ca4b7081c272ca01d386ed7962e66f6ee
Author: Matt Gilman <ma...@...>
Date: 2018-05-18T18:56:10Z
NIFI-5186:
- Updating UI to support showing when a component is validating.
----
---
[GitHub] nifi issue #2722: NIFI-5186: Updating UI to support asynchronous validation
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/2722
@joewitt re: the unit test failure, I don't believe that it's actually related to the previous PR necessarily, but is just a timing issue that happened to trigger here. We could certainly raise the timeout used - it was waiting up to 2 seconds for the processor state to become STOPPED in the unit test. We could certainly raise that to 10 seconds or so. I think the timeout chosen was fairly arbitrary and increasing it would only increase the amount of time that the tests takes to complete when needed to. If the state transitions to STOPPED after 25 milliseconds then that's all that it'll wait.
---
[GitHub] nifi issue #2722: NIFI-5186: Updating UI to support asynchronous validation
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2722
@mcgilman @markap14 this test finding is related to Mark's preceding PR for this most likely but
`[ERROR] Tests run: 15, Failures: 1, Errors: 0, Skipped: 2, Time elapsed: 16.555 s <<< FAILURE! - in org.apache.nifi.controller.scheduling.TestProcessorLifecycle
[ERROR] validateProcessorCanBeStoppedWhenOnTriggerThrowsException(org.apache.nifi.controller.scheduling.TestProcessorLifecycle) Time elapsed: 3.11 s <<< FAILURE!
java.lang.AssertionError
at org.apache.nifi.controller.scheduling.TestProcessorLifecycle.assertCondition(TestProcessorLifecycle.java:116)
at org.apache.nifi.controller.scheduling.TestProcessorLifecycle.validateProcessorCanBeStoppedWhenOnTriggerThrowsException(TestProcessorLifecycle.java:496)
`
Otherwise, I'm doing some functional testing now and obviously contrib-check is good. This is really cool and will greatly aid flow performance for large scale flows or user code in validation that doesn't behave nicely.
---
[GitHub] nifi pull request #2722: NIFI-5186: Updating UI to support asynchronous vali...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2722#discussion_r189701115
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/main/java/org/apache/nifi/cluster/manager/StatusMerger.java ---
@@ -415,13 +415,15 @@ public static void merge(final ProcessorStatusSnapshotDTO target, final boolean
target.setType(toMerge.getType());
}
- // if the status to merge is invalid allow it to take precedence. whether the
+ // if the status to merge is validating/invalid allow it to take precedence. whether the
// processor run status is disabled/stopped/running is part of the flow configuration
- // and should not differ amongst nodes. however, whether a processor is invalid
+ // and should not differ amongst nodes. however, whether a processor is validating/invalid
// can be driven by environmental conditions. this check allows any of those to
// take precedence over the configured run status.
- if (RunStatus.Invalid.name().equals(toMerge.getRunStatus())) {
- target.setRunStatus(RunStatus.Invalid.name());
+ if (RunStatus.Validating.toString().equals(toMerge.getRunStatus())) {
--- End diff --
I was just addressing an inconsistency because in the `DtoFactory` we populate run status with the value from `toString()`.
---
[GitHub] nifi issue #2722: NIFI-5186: Updating UI to support asynchronous validation
Posted by scottyaslan <gi...@git.apache.org>.
Github user scottyaslan commented on the issue:
https://github.com/apache/nifi/pull/2722
Thanks @mcgilman this has been merged to master.
---
[GitHub] nifi issue #2722: NIFI-5186: Updating UI to support asynchronous validation
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/2722
@joewitt I think I understand now what you were seeing. If I create two different DebugFlow processors, each with a validation pause of 10 seconds, and then I create an UpdateAttribute and configure each of them in quick succession, I see UpdateAttribute in a 'Validating' state for many seconds. I have created a new JIRA for this, though, NIFI-5222, as I don't think it's something that should really block merging this PR in. The issue appears to be that we kick off validation 3 times each time that we click "Update" and so if we update both DebugFlow processors, we end up kicking off 6 validation tasks instead of 2, and that ends up blocking UpdateAttribute since we have only 5 threads in the pool.
---
[GitHub] nifi pull request #2722: NIFI-5186: Updating UI to support asynchronous vali...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2722#discussion_r189922253
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-controller-services.js ---
@@ -814,12 +814,20 @@
var bType = nfCommon.isDefinedAndNotNull(b.component[sortDetails.columnId]) ? nfCommon.substringAfterLast(b.component[sortDetails.columnId], '.') : '';
return aType === bType ? 0 : aType > bType ? 1 : -1;
} else if (sortDetails.columnId === 'state') {
- var aState = 'Invalid';
- if (nfCommon.isEmpty(a.component.validationErrors)) {
+ var aState;
+ if (a.component.validationStatus === 'VALIDATING') {
+ aState = 'Validating';
+ } else if (a.component.validationStatus === 'INVALID') {
+ aState = 'Invalid';
+ } else {
aState = nfCommon.isDefinedAndNotNull(a.component[sortDetails.columnId]) ? a.component[sortDetails.columnId] : '';
}
- var bState = 'Invalid';
- if (nfCommon.isEmpty(b.component.validationErrors)) {
+ var bState;
+ if (a.component.validationStatus === 'VALIDATING') {
--- End diff --
Yup lemme update.
---
[GitHub] nifi pull request #2722: NIFI-5186: Updating UI to support asynchronous vali...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2722
---
[GitHub] nifi issue #2722: NIFI-5186: Updating UI to support asynchronous validation
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/2722
@joewitt your observations are very interesting - i am not seeing that at all. I created a DebugFlow with a validation pause of 10 seconds. Then I created an update attribute. UpdateAttribute was immediately invalid. I triggering validation to occur on DebugFlow and then UpdateAttribute and saw instance responses for UpdateAttribute's validation.
So to answer your questions specifically:
1) We use a thread pool with 5 threads.
2) Any time a component changes (specifically, if a property is set, annotation data is set, incoming connections or outgoing connections are set) then we initiate validation for the component.
Of note, when you get back the "Validating" icon, that icon will not go away until the status is refreshed, which happens by default every 30 seconds. If you right-click on the canvas and refresh, that will also refresh it. Just wanted to make sure that you did refresh the canvas in that time interval?
---
[GitHub] nifi issue #2722: NIFI-5186: Updating UI to support asynchronous validation
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2722
@markap14 @mcgilman in looking at this from a user point of view a couple thoughts:
When validating things which are slow and things which are fast it appears fast things can be bottlenecked behind fast things. This makes sense but can we do better? For instance I have a DebugFlow with a 10 sec validation cycle and an UpdateAttribute processor. it took 10 secs or more for UpdateAttribute to come back as valid presumably because it was waiting for the next validation cycle and then perhaps it had to wait behind the slow DebugFlow proc.
Question: Do we have 1 thread or more than one thread for validation? Or is it just a task we fire off in the typical timer task thread pool?
Question: Can we force a validation cycle whenever some component is change or more to the point could we initiate 'that component' which changed to be validated.
---
[GitHub] nifi pull request #2722: NIFI-5186: Updating UI to support asynchronous vali...
Posted by scottyaslan <gi...@git.apache.org>.
Github user scottyaslan commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2722#discussion_r189688839
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-controller-services.js ---
@@ -814,12 +814,20 @@
var bType = nfCommon.isDefinedAndNotNull(b.component[sortDetails.columnId]) ? nfCommon.substringAfterLast(b.component[sortDetails.columnId], '.') : '';
return aType === bType ? 0 : aType > bType ? 1 : -1;
} else if (sortDetails.columnId === 'state') {
- var aState = 'Invalid';
- if (nfCommon.isEmpty(a.component.validationErrors)) {
+ var aState;
+ if (a.component.validationStatus === 'VALIDATING') {
+ aState = 'Validating';
+ } else if (a.component.validationStatus === 'INVALID') {
+ aState = 'Invalid';
+ } else {
aState = nfCommon.isDefinedAndNotNull(a.component[sortDetails.columnId]) ? a.component[sortDetails.columnId] : '';
}
- var bState = 'Invalid';
- if (nfCommon.isEmpty(b.component.validationErrors)) {
+ var bState;
+ if (a.component.validationStatus === 'VALIDATING') {
--- End diff --
should this be checking b.component.validationStatus?
---
[GitHub] nifi pull request #2722: NIFI-5186: Updating UI to support asynchronous vali...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2722#discussion_r189922272
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-settings.js ---
@@ -264,12 +264,20 @@
var bType = nfCommon.isDefinedAndNotNull(b.component[sortDetails.columnId]) ? nfCommon.substringAfterLast(b.component[sortDetails.columnId], '.') : '';
return aType === bType ? 0 : aType > bType ? 1 : -1;
} else if (sortDetails.columnId === 'state') {
- var aState = 'Invalid';
- if (nfCommon.isEmpty(a.component.validationErrors)) {
+ var aState;
+ if (a.component.validationStatus === 'VALIDATING') {
+ aState = 'Validating';
+ } else if (a.component.validationStatus === 'INVALID') {
+ aState = 'Invalid';
+ } else {
aState = nfCommon.isDefinedAndNotNull(a.component[sortDetails.columnId]) ? a.component[sortDetails.columnId] : '';
}
- var bState = 'Invalid';
- if (nfCommon.isEmpty(b.component.validationErrors)) {
+ var bState;
+ if (a.component.validationStatus === 'VALIDATING') {
+ bState = 'Validating';
--- End diff --
Yup lemme update.
---
[GitHub] nifi issue #2722: NIFI-5186: Updating UI to support asynchronous validation
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2722
@markap14 understood and agree. I'm +1 on this. Will also look at NIFI-5222 as well now
---
[GitHub] nifi pull request #2722: NIFI-5186: Updating UI to support asynchronous vali...
Posted by scottyaslan <gi...@git.apache.org>.
Github user scottyaslan commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2722#discussion_r189689454
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-settings.js ---
@@ -264,12 +264,20 @@
var bType = nfCommon.isDefinedAndNotNull(b.component[sortDetails.columnId]) ? nfCommon.substringAfterLast(b.component[sortDetails.columnId], '.') : '';
return aType === bType ? 0 : aType > bType ? 1 : -1;
} else if (sortDetails.columnId === 'state') {
- var aState = 'Invalid';
- if (nfCommon.isEmpty(a.component.validationErrors)) {
+ var aState;
+ if (a.component.validationStatus === 'VALIDATING') {
+ aState = 'Validating';
+ } else if (a.component.validationStatus === 'INVALID') {
+ aState = 'Invalid';
+ } else {
aState = nfCommon.isDefinedAndNotNull(a.component[sortDetails.columnId]) ? a.component[sortDetails.columnId] : '';
}
- var bState = 'Invalid';
- if (nfCommon.isEmpty(b.component.validationErrors)) {
+ var bState;
+ if (a.component.validationStatus === 'VALIDATING') {
+ bState = 'Validating';
--- End diff --
should this be checking b.component.validationStatus
---
[GitHub] nifi pull request #2722: NIFI-5186: Updating UI to support asynchronous vali...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2722#discussion_r189656654
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/main/java/org/apache/nifi/cluster/manager/StatusMerger.java ---
@@ -415,13 +415,15 @@ public static void merge(final ProcessorStatusSnapshotDTO target, final boolean
target.setType(toMerge.getType());
}
- // if the status to merge is invalid allow it to take precedence. whether the
+ // if the status to merge is validating/invalid allow it to take precedence. whether the
// processor run status is disabled/stopped/running is part of the flow configuration
- // and should not differ amongst nodes. however, whether a processor is invalid
+ // and should not differ amongst nodes. however, whether a processor is validating/invalid
// can be driven by environmental conditions. this check allows any of those to
// take precedence over the configured run status.
- if (RunStatus.Invalid.name().equals(toMerge.getRunStatus())) {
- target.setRunStatus(RunStatus.Invalid.name());
+ if (RunStatus.Validating.toString().equals(toMerge.getRunStatus())) {
--- End diff --
It's probably best to use .name() instead of .toString(), as toString() could potentially change at any time, whereas the name never will
---