You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "timeabarna (via GitHub)" <gi...@apache.org> on 2023/06/07 13:20:01 UTC
[GitHub] [nifi] timeabarna opened a new pull request, #7353: NIFI-11658 Streamline using single parameter context for nested PGs
timeabarna opened a new pull request, #7353:
URL: https://github.com/apache/nifi/pull/7353
# Summary
[NIFI-11658](https://issues.apache.org/jira/browse/NIFI-11658)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [ ] Pull Request based on current revision of the `main` branch
- [ ] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7353:
URL: https://github.com/apache/nifi/pull/7353#discussion_r1261613603
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/process-group-configuration.jsp:
##########
@@ -40,6 +40,12 @@
<div class="setting-name">Process group parameter context</div>
<div class="editable setting-field">
<div id="process-group-parameter-context-combo"></div>
+ <div id="parameter-contexts-recursive-container">
+ <div id="parameter-contexts-recursive" class="nf-checkbox checkbox-unchecked"></div>
+ <div class="nf-checkbox-label">Apply recursively</div>
Review Comment:
The positioning of this checkbox seems out of place in comparison to the other form elements. One option could be to place it directly under the `Process Group Parameter Context` select field.
However, another complicating factor is that unlike the other settings, this setting is not persistent. In other words, if a user checks the box and clicks the `Apply` button, the box will be unchecked when returning to the screen. This seems like it could create some confusion for users, especially if the UI gives the impression that Parameter Context selection will always be applied to any nested Process Groups.
With that background, another option could be to move this checkbox field right above, or next to the `Apply` button. That would probably warrant changing the labeling, because then it would appear to apply to all settings, instead of just the Parameter Context.
Yet one more would be to introduce a new tab in the Process Group configuration dialog specific to Parameter Contexts. That would also help differentiate permanent settings from temporary options like this option to apply Parameter Contexts recursively.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7353:
URL: https://github.com/apache/nifi/pull/7353#discussion_r1242201390
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-client-dto/src/main/java/org/apache/nifi/web/api/entity/ProcessGroupUpdateStrategy.java:
##########
@@ -0,0 +1,22 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.api.entity;
+
+public enum ProcessGroupUpdateStrategy {
+ UPDATE_PROCESS_GROUP_ONLY,
+ UPDATE_PROCESS_GROUP_WITH_DESCENDANTS
Review Comment:
Instead of repeating the `UPDATE_PROCESS_GROUP` prefix, recommend changing the naming to be more concise. What about `CURRENT_GROUP` and `CURRENT_GROUP_WITH_CHILDREN`? Much of User Guide documentation mentions "Child Process Groups".
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/NiFiServiceFacade.java:
##########
@@ -1209,9 +1209,10 @@ Set<DocumentedTypeDTO> getControllerServiceTypes(final String serviceType, final
* Gets all process groups in the specified parent group.
*
* @param parentGroupId The id of the parent group
- * @return process group
+ * @param includeDescendants if process groups from descendant groups should be included
+ * @return List of process groups
*/
- Set<ProcessGroupEntity> getProcessGroups(String parentGroupId);
+ Set<ProcessGroupEntity> getProcessGroups(String parentGroupId, boolean includeDescendants);
Review Comment:
Instead of using a `boolean` here, why not pass the `ProcessGroupUpdateStrategy`?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/ProcessGroupDAO.java:
##########
@@ -59,9 +59,10 @@ public interface ProcessGroupDAO {
* Gets all of the process groups.
*
* @param parentGroupId The parent group id
+ * @param includeDescendants if process groups from descendant groups should be included
* @return The process groups
*/
- Set<ProcessGroup> getProcessGroups(String parentGroupId);
+ Set<ProcessGroup> getProcessGroups(String parentGroupId, boolean includeDescendants);
Review Comment:
See above note on using the `ProcessGroupUpdateStrategy` enum.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mcgilman commented on a diff in pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
Posted by "mcgilman (via GitHub)" <gi...@apache.org>.
mcgilman commented on code in PR #7353:
URL: https://github.com/apache/nifi/pull/7353#discussion_r1224527610
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/process-group-configuration.jsp:
##########
@@ -109,6 +100,34 @@
<div id="process-group-controller-services-tab-content" class="configuration-tab">
<div id="process-group-controller-services-table" class="settings-table"></div>
</div>
+ <div id="process-group-parameter-contexts-tab-content" class="configuration-tab">
Review Comment:
What additional configuration elements for the current Process Group's referenced Parameter Context are expected?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on PR #7353:
URL: https://github.com/apache/nifi/pull/7353#issuecomment-1608890622
Hello @exceptionfactory ,
Thank you ver much for your review, I've updated the PR based on your recommendation.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on PR #7353:
URL: https://github.com/apache/nifi/pull/7353#issuecomment-1651801814
Bumping to prevent stale status
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory closed pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory closed pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
URL: https://github.com/apache/nifi/pull/7353
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on a diff in pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on code in PR #7353:
URL: https://github.com/apache/nifi/pull/7353#discussion_r1262121812
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/process-group-configuration.jsp:
##########
@@ -40,6 +40,12 @@
<div class="setting-name">Process group parameter context</div>
<div class="editable setting-field">
<div id="process-group-parameter-context-combo"></div>
+ <div id="parameter-contexts-recursive-container">
+ <div id="parameter-contexts-recursive" class="nf-checkbox checkbox-unchecked"></div>
+ <div class="nf-checkbox-label">Apply recursively</div>
Review Comment:
Thanks @exceptionfactory for your insights.
The original idea was to have a dedicated tab for Parameter Context however after a discussion this was dropped as the current UI implementation is not applicable for this. I think the main concern is that having Apply buttons at two different contexts (Process Group Configuration and Parameter Context) require some extra validations and checks which would probably introduce some regressions.
The agreement wast to add the Apply recursive button next to the Parameter Context selection field and as there are some more new additions to the Process Group Configuration screen, such as Per Process Group Logging and some stateless related configurations, have a separate PR for rearranging the screen.
The new screen planed to be similar to the Parameter Provider screen with multiple columns where configuration fields grouped together based on some logic which needs to be discussed as part of a different activity.
@mcgilman , @mtien-apache and @sardell please feel free to chime in and share your thoughts.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on a diff in pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on code in PR #7353:
URL: https://github.com/apache/nifi/pull/7353#discussion_r1224039695
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/process-group-configuration.jsp:
##########
@@ -109,6 +100,34 @@
<div id="process-group-controller-services-tab-content" class="configuration-tab">
<div id="process-group-controller-services-table" class="settings-table"></div>
</div>
+ <div id="process-group-parameter-contexts-tab-content" class="configuration-tab">
Review Comment:
Hello @mcgilman ,
Thank you for your review.
We are expecting to add more configuration elements to Parameter Context, moving Parameter Context modifications to a separate tab makes Process Group Configuration window less overloaded.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mtien-apache commented on pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
Posted by "mtien-apache (via GitHub)" <gi...@apache.org>.
mtien-apache commented on PR #7353:
URL: https://github.com/apache/nifi/pull/7353#issuecomment-1584771604
Reviewing.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mcgilman commented on a diff in pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
Posted by "mcgilman (via GitHub)" <gi...@apache.org>.
mcgilman commented on code in PR #7353:
URL: https://github.com/apache/nifi/pull/7353#discussion_r1223267396
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/new-process-group-dialog.jsp:
##########
@@ -28,6 +28,12 @@
</div>
<input id="new-process-group-name" type="text" placeholder="Enter a name or select a file to upload"/>
</div>
+ </div>
+ <div class="setting">
+ <div class="setting-name">Parameter Context</div>
Review Comment:
The changes to the new Process Group Dialog should warrant increasing the vertical height.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/process-group-configuration.jsp:
##########
@@ -109,6 +100,34 @@
<div id="process-group-controller-services-tab-content" class="configuration-tab">
<div id="process-group-controller-services-table" class="settings-table"></div>
</div>
+ <div id="process-group-parameter-contexts-tab-content" class="configuration-tab">
Review Comment:
What was the motivation for moving this configuration into a new tab?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/header/components/nf-ng-group-component.js:
##########
@@ -127,6 +130,67 @@
return filepath.replace(/^.*[\\\/]/, '').replace(/\..*/, '');
};
+ /**
+ * parameter context
+ */
+ var parameterContextOptions = [];
+ var noParameterContext = {
+ text: 'No parameter context',
+ value: null
+ };
+ var selectedOption = noParameterContext;
+ var parentParameterContextUrl = '../nifi-api/flow/process-groups/' + encodeURIComponent(nfCanvasUtils.getGroupId());
Review Comment:
I think there is a bug here. I think this code is evaluated once when this loads. And current Process Group Id will be different based on when and where a new Process Group is being added. We should determine this URL when the Process Group is being added. I think we should limit variables here to things that are constants and do not change based on the current context.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -123,6 +175,14 @@
dataType: 'json',
contentType: 'application/json'
}).done(function (response) {
+ // check parameter context recursive update requirement
+ if ($('#parameter-contexts-recursive').hasClass('checkbox-checked')) {
+ var parameterContextId = $('#process-group-parameter-context-combo').combo('getSelectedOption').value;
+ allDescendantProcessGroups.forEach(function (processGroup) {
+ updateParameterContext(processGroup.revision.version, processGroup.id, parameterContextId);
Review Comment:
What happens if one of these calls fails? Is there a reason why the `recursive` flag isn't passed into the call update the current Process Group?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/header/components/nf-ng-group-component.js:
##########
@@ -349,11 +416,15 @@
}
});
+ getParameterContextOptions();
Review Comment:
This should return a `deferred` with the options so we don't execute the following code until it completes.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/header/components/nf-ng-group-component.js:
##########
@@ -127,6 +130,67 @@
return filepath.replace(/^.*[\\\/]/, '').replace(/\..*/, '');
};
+ /**
+ * parameter context
+ */
+ var parameterContextOptions = [];
Review Comment:
I would also suggest we move this variable out of this scope and instead return it from `getParameterContextOptions`. The function could return a `deferred` which provides the populated array.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -123,6 +175,14 @@
dataType: 'json',
contentType: 'application/json'
}).done(function (response) {
+ // check parameter context recursive update requirement
+ if ($('#parameter-contexts-recursive').hasClass('checkbox-checked')) {
+ var parameterContextId = $('#process-group-parameter-context-combo').combo('getSelectedOption').value;
+ allDescendantProcessGroups.forEach(function (processGroup) {
+ updateParameterContext(processGroup.revision.version, processGroup.id, parameterContextId);
Review Comment:
Applying the Parameter Context change recursively does not work when the user lacks permission to any Parameter Contexts referenced by any descendent Process Groups. Currently, the UI reports that everything was saved successfully. But you can see there are requests failing with 403.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -513,6 +552,20 @@
// initialize the parameter context combo
$('#process-group-parameter-context-combo').combo('destroy').combo(comboOptions);
+
+ // initialize parameter context recursive checkbox
+ getAllDescendantProcessGroup(groupId).then(function (response) {
Review Comment:
Making this call when the user does not have permissions to the current Process Group, results in an error in the UI. Currently, we should be handling this without a user-facing error because the user may lack permissions for the current PG but have explicit permissions for a Controller Service in the listing.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -85,6 +85,58 @@
return $('#process-group-controller-services-table');
};
+ /**
+ *
+ * Gets all process groups of a process group
+ * @param groupId
+ * @returns process groups array
+ **/
+ var allDescendantProcessGroups;
Review Comment:
It would be preferable if this was retrieved when needed or passed in as an argument rather then storing it here.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -123,6 +175,14 @@
dataType: 'json',
contentType: 'application/json'
}).done(function (response) {
+ // check parameter context recursive update requirement
+ if ($('#parameter-contexts-recursive').hasClass('checkbox-checked')) {
+ var parameterContextId = $('#process-group-parameter-context-combo').combo('getSelectedOption').value;
+ allDescendantProcessGroups.forEach(function (processGroup) {
+ updateParameterContext(processGroup.revision.version, processGroup.id, parameterContextId);
Review Comment:
Applying the Parameter Context change recursively does not work when the user lacks permissions to any descendent Process Group. The UI here also reported success but there was a failed request with 500. The corresponding stack trace was:
```
15 java.lang.NullPointerException: null
16 at org.apache.nifi.web.api.ProcessGroupResource.lambda$updateProcessGroup$4(ProcessGroupResource.java:561)
17 at org.apache.nifi.web.StandardNiFiServiceFacade.authorizeAccess(StandardNiFiServiceFacade.java:468)
18 at org.apache.nifi.web.StandardNiFiServiceFacade$$FastClassBySpringCGLIB$$358780e0.invoke(<generated>)
19 at org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:218)
```
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on pull request #7353: NIFI-11658 Streamline using single parameter context for nested PGs
Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on PR #7353:
URL: https://github.com/apache/nifi/pull/7353#issuecomment-1614151688
Thanks @exceptionfactory and @mcgilman for your review, I've updated the PR.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org