NIFI-401 #1117

beugley · 2016-10-07T22:29:30Z

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:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
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)?
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?
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?
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.

mcgilman · 2016-10-19T17:12:00Z

@beugley Thanks for the PR! Going to start reviewing... Sorry this lingered a bit. I see there are a couple conflicting files but I'm going to see if I can rebase them locally. If I have any issues, I'll reach back out.

mcgilman · 2016-10-19T18:17:09Z

...k-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/css/processor-configuration.css

@@ -110,6 +110,12 @@ div.processor-enabled-container {
    line-height: 18px;
 }

+#execution-node-combo {


With the updated UI in 1.0.0 I don't think this style is necessary anymore. Specifically the width and height are not consistent with the other combos in the scheduling tab.

mcgilman · 2016-10-19T18:31:21Z

...fi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-processor-configuration.js

-                value: 'PRIMARY_NODE_ONLY',
-                description: 'Processor will be scheduled on the primary node on an interval defined by the run schedule.'
-            });
-        } else if (processor.config['schedulingStrategy'] === 'PRIMARY_NODE_ONLY') {


I realize that we are deprecating 'Primary Node Only' but I believe we still need to retain this case to handle existing components with this configuration. The disabled flag will prevent this option from being selected again.

mcgilman · 2016-10-19T18:31:27Z

...fi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-processor-configuration.js

@@ -201,6 +185,9 @@ nf.ProcessorConfiguration = (function () {
            return true;
        }

+        if ($('#execution-node-combo').combo('getSelectedOption').value !== (details.config['executionNode'] + '')) {


Should we be considering whether the execution node combo is available before using it to determine if a save is required?

mcgilman · 2016-10-19T18:31:34Z

...fi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-processor-configuration.js

+                    // the dialog is applied.
+                    var schedulingStrategy = processor.config['schedulingStrategy'];
+                    var executionNode = processor.config['executionNode'];
+                    if (schedulingStrategy === 'PRIMARY_NODE_ONLY') {


This appears to be rendering the inferred configuration based on the currently (now deprecated) option. I think we should be rendering what is actually configured. If the option is now deprecated we should mark the item as disabled to prevent it from being selected again. But I do not believe we should be rendering something that is not actually configured.

mcgilman · 2016-10-19T18:37:27Z

...fi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-processor-configuration.js

+                    $('#execution-node-combo').combo('setSelectedOption', {
+                        value: executionNode
+                    });
+                    if (nf.Canvas.isClustered()) {


If we are going to hide/show the execution node container, we should probably also be considering the currently configured value. If a node is configured for 'Primary Node Only' we need to retain that fact even when not clustered. If a user has disconnected a node and then went to that node directly we still want the configuration actually reflected. Should probably show the field if clustered or if the execution node is 'primary node only'.

mcgilman · 2016-10-19T18:55:15Z

...ork/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/processor-configuration.jsp

+                        <div id="execution-node-container" class="execution-node-setting">
+                            <div class="setting-name">
+                                Execution node
+                                <img class="setting-icon icon-info" src="images/iconInfo.png" alt="Info" title="The node(s) that will run this processor."/>


Can you update the 'Info Icon' to use the same styles as the others in the scheduling tab?

mcgilman · 2016-10-19T18:56:43Z

nifi-api/src/main/java/org/apache/nifi/scheduling/SchedulingStrategy.java

+     * NOTE: This option has been deprecated with the addition of the
+     * execution-node combo box.  It still exists for backward compatibility
+     * with existing flows that still have this value for schedulingStrategy.
+     **


Probably should actually mark this option as deprecated using an annotation.

mcgilman · 2016-10-19T19:12:51Z

...ork/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/processor-configuration.jsp

@@ -108,6 +108,18 @@
                        </div>
                        <div class="clear"></div>
                    </div>
+                    <div class="setting">


The Scheduling Strategy and the Run Schedule should probably remain closer together. Can you shift the Execution Node setting below the Concurrent Tasks and Run Schedule.

mcgilman · 2016-10-19T19:17:57Z

...e/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/processor-details.jsp

@@ -103,6 +103,18 @@
                        <div class="clear"></div>
                    </div>
                    <div class="setting">
+                        <div id="details-execution-node-container" class="execution-node-setting">


Same comment as above, we should keep the Scheduling Strategy and Run Schedule close together by shifting the Execution Node below the Concurrent Tasks and Run Schedule.

Thanks for considering the PR. Seems like there are lots of problems with the code. If you want to take over, please do so, but at this point I'm not inclined to continue working on this feature.

No worries. I don't think there are a lot of problems with the code. I think what was provided is quite good and really close. Most comments are mostly based on the same premise of retaining the existing configuration until a user has explicitly configured a Processor using the new options. I just tried to mark each spot where we should consider that.

Either way, I am happy to continue working with you on this PR or I would be happy to pick up at this point if you'd prefer.

Thanks again!

yep, as you noted my goal was to accept processors with older configurations, and then update to a newer configuration when the user changed the processor in any form. At the moment, I'm very busy with other tasks, so don't have time to work on this now, and won't in the near future. If you want to pick this up, go ahead, I won't be offended in any way. Thanks again for reviewing it and considering the change!
Brian

Awesome. Will do!

I will leave this PR open and I will issue another PR with this commit and any subsequent work. I'll close out both PRs once everything is merged in.

Thanks again!

mcgilman · 2016-10-19T19:22:35Z

...fi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-processor-configuration.js

@@ -264,6 +251,7 @@ nf.ProcessorConfiguration = (function () {
            processorConfigDto['schedulingPeriod'] = schedulingPeriod.val();
        }

+        processorConfigDto['executionNode'] = $('#execution-node-combo').combo('getSelectedOption').value;


Should probably consider whether the execution node is available before including its value in the processor's configuration.

- Minor tweaks to PR apache#1117.

- Minor tweaks to PR apache#1117. - Ensuring existing configuraiton is retained and shown until the user explicits changes it. - Retaining, but disabling, deprecated options.

NIFI-401

011b5e7

mcgilman reviewed Oct 19, 2016

View reviewed changes

mcgilman added a commit to mcgilman/nifi that referenced this pull request Oct 31, 2016

NIFI-401:

a579bb4

- Minor tweaks to PR apache#1117.

mcgilman added a commit to mcgilman/nifi that referenced this pull request Nov 1, 2016

NIFI-401:

d405538

- Minor tweaks to PR apache#1117. - Ensuring existing configuraiton is retained and shown until the user explicits changes it. - Retaining, but disabling, deprecated options.

mcgilman added a commit to mcgilman/nifi that referenced this pull request Nov 4, 2016

NIFI-401:

6c1ef9a

- Minor tweaks to PR apache#1117. - Ensuring existing configuraiton is retained and shown until the user explicits changes it. - Retaining, but disabling, deprecated options.

mcgilman added a commit to mcgilman/nifi that referenced this pull request Nov 4, 2016

NIFI-401:

7f8d4ab

- Minor tweaks to PR apache#1117. - Ensuring existing configuraiton is retained and shown until the user explicits changes it. - Retaining, but disabling, deprecated options.

mcgilman mentioned this pull request Nov 4, 2016

NIFI-401: Decouple Scheduling Strategy from Execution Node #1185

Closed

asfgit closed this in bff89f1 Nov 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-401 #1117

NIFI-401 #1117

beugley commented Oct 7, 2016

mcgilman commented Oct 19, 2016

mcgilman Oct 19, 2016

mcgilman Oct 19, 2016

mcgilman Oct 19, 2016

mcgilman Oct 19, 2016

mcgilman Oct 19, 2016

mcgilman Oct 19, 2016

mcgilman Oct 19, 2016

mcgilman Oct 19, 2016

mcgilman Oct 19, 2016

beugley Oct 20, 2016

mcgilman Oct 20, 2016

beugley Oct 21, 2016

mcgilman Oct 21, 2016

mcgilman Oct 19, 2016

NIFI-401 #1117

NIFI-401 #1117

Conversation

beugley commented Oct 7, 2016

For all changes:

For code changes:

For documentation related changes:

Note:

mcgilman commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment