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 2022/01/18 14:54:10 UTC
[GitHub] [nifi] sardell opened a new pull request #5666: [WIP] NIFI-9580: UI work for framework-level retry in Processors
sardell opened a new pull request #5666:
URL: https://github.com/apache/nifi/pull/5666
<!--
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.
-->
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
This is an initial pass at adding UI controls for the retry work done in [#5593](https://github.com/apache/nifi/pull/5593).
Pending TODOs:
* Disable retry controls if no retry checkboxes are checked
* Styling tweaks
_Enables X functionality; fixes bug NIFI-YYYY._
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 `main`)?
- [ ] 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:
- [ ] 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?
- [ ] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] 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 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.
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 change in pull request #5666: NIFI-9580: UI work for framework-level retry in Processors
Posted by GitBox <gi...@apache.org>.
mcgilman commented on a change in pull request #5666:
URL: https://github.com/apache/nifi/pull/5666#discussion_r809367895
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/processor-configuration.jsp
##########
@@ -223,6 +212,52 @@
<div id="processor-properties-verification-results-listing" class="verification-results-listing"></div>
</div>
</div>
+ <div id="processor-relationships-tab-content" class="configuration-tab">
+ <div class="settings-left">
+ <div class="setting">
+ <div class="setting-name">
+ Automatically terminate / retry relationships
+ <div class="fa fa-question-circle" alt="Info" title="Will automatically terminate or retry FlowFiles sent to a given relationship if it is not defined elsewhere."></div>
Review comment:
Configuring both retry and auto-terminate is a valid configuration. For instance, retry 10 times then terminate. The tooltip as-is suggests it's one or the other. We should clarify that both is valid and that the retry logic happens before auto-termination.
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-processor-configuration.js
##########
@@ -333,7 +367,11 @@
}
// relationships
- processorConfigDto['autoTerminatedRelationships'] = marshalRelationships();
+ processorConfigDto['autoTerminatedRelationships'] = marshalRelationships('terminate');
+ processorConfigDto['retriedRelationships'] = marshalRelationships('retry');
Review comment:
If there are no relationships being retried, the inputs defining the retry criteria are hidden. However, below we are always including them in the request payload. So if the user changes the retry criteria and then unselects the retry checkbox for all relationships, the inputs are hidden but the new values are saved.
I believe we have the same scenario for Processors that support Event Driven scheduling strategy. If scheduling strategy was Timer Driven and the user opens the configuration dialog and changes the Run Schedule and switches to Event Driven scheduling strategy the Run Schedule control is hidden but the new value isn't included in the request payload.
--
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] sardell commented on a change in pull request #5666: NIFI-9580: UI work for framework-level retry in Processors
Posted by GitBox <gi...@apache.org>.
sardell commented on a change in pull request #5666:
URL: https://github.com/apache/nifi/pull/5666#discussion_r816220189
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-processor-configuration.js
##########
@@ -333,7 +367,11 @@
}
// relationships
- processorConfigDto['autoTerminatedRelationships'] = marshalRelationships();
+ processorConfigDto['autoTerminatedRelationships'] = marshalRelationships('terminate');
+ processorConfigDto['retriedRelationships'] = marshalRelationships('retry');
Review comment:
@mcgilman Thanks for pointing this out. I added this bit of logic to only send retry-associated values in the payload if a relationship is selected for retry: https://github.com/apache/nifi/pull/5666/files#diff-ddf6a79c16963065d09b6e220fade7bcc444a155b0f3a91e6585f097440ad112R376-R380
--
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] sardell commented on pull request #5666: NIFI-9580: UI work for framework-level retry in Processors
Posted by GitBox <gi...@apache.org>.
sardell commented on pull request #5666:
URL: https://github.com/apache/nifi/pull/5666#issuecomment-1040872242
@markap14 Thanks for taking a look! I updated the language used in the tooltips, and the default values you recommended are now the default returned from the backend (thanks @timeabarna!).
--
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] sardell commented on a change in pull request #5666: [WIP] NIFI-9580: UI work for framework-level retry in Processors
Posted by GitBox <gi...@apache.org>.
sardell commented on a change in pull request #5666:
URL: https://github.com/apache/nifi/pull/5666#discussion_r786927030
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-processor-configuration.js
##########
@@ -214,6 +235,28 @@
return true;
}
+ // consider retried relationships
Review comment:
There's an opportunity to keep things DRY here with the `marshalAutoTerminateRelationships` and `marshalRetriedRelationships` by creating one method that takes an argument to determine whether we are marshaling auto terminate or retry. Will update this when I get a chance.
--
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] markap14 commented on pull request #5666: NIFI-9580: UI work for framework-level retry in Processors
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #5666:
URL: https://github.com/apache/nifi/pull/5666#issuecomment-1035444808
Thanks for putting up the Draft request here, Shane! I've been playing around with it, and I think design works well. A few recommendations, though, around tooltips and defaults to help simplify the configuration and make things more seamless for others:
- For "Number of Retry Attempts" I would recommend adding a tooltip that reads something along the lines of: "If a FlowFile is routed to one of the Relationships that is selected for retry, this indicates how many times the FlowFile will be retried before being routed to the associated Relationship. The total number of attempts will be one more than the configured value of Retry attempts." Perhaps a bit less wordy :)
- Also for the backoff policy I would recommend a tooltip that explains the difference. When penalizing, the retried FlowFile will be retried after some time, but the processor will process other FlowFiles in the meantime. However, if FlowFile ordering is necessary, Yielding can be used ensure that no other FlowFiles are processed until the first one has completed all retries.
- Would also recommend the verbiage "yield processor" rather than "yield entire processor"
- re: "Retry Max Backoff Period" - the first time a FlowFile is retried, it will be penalized - or the processor yielded - based on the Penalty Duration / Yield Duration configured in the Settings tab. The second time, the duration will be doubled. And then doubled again, up until the Max Backoff Period. Should probably have a tooltip to explain this also.
- I would recommend "Number of Retry Attempts" not default to 0. Perhaps 3 or 5 or 10. Maybe 10 retries is a reasonable default.
- Recommend a Max Backoff Period of "10 mins" instead of "30 secs" - 30 secs is a good starting value but I think 10 mins is probably more appropriate for the max amount of time.
--
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] sardell commented on a change in pull request #5666: NIFI-9580: UI work for framework-level retry in Processors
Posted by GitBox <gi...@apache.org>.
sardell commented on a change in pull request #5666:
URL: https://github.com/apache/nifi/pull/5666#discussion_r813238649
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/processor-configuration.jsp
##########
@@ -223,6 +212,52 @@
<div id="processor-properties-verification-results-listing" class="verification-results-listing"></div>
</div>
</div>
+ <div id="processor-relationships-tab-content" class="configuration-tab">
+ <div class="settings-left">
+ <div class="setting">
+ <div class="setting-name">
+ Automatically terminate / retry relationships
+ <div class="fa fa-question-circle" alt="Info" title="Will automatically terminate or retry FlowFiles sent to a given relationship if it is not defined elsewhere."></div>
Review comment:
Updated this to say, "Will automatically terminate and/or retry FlowFiles sent to a given relationship if it is not defined elsewhere. If both terminate and retry are selected, any retry logic will happen first, then auto-termination."
--
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 merged pull request #5666: NIFI-9580: UI work for framework-level retry in Processors
Posted by GitBox <gi...@apache.org>.
mcgilman merged pull request #5666:
URL: https://github.com/apache/nifi/pull/5666
--
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