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 2021/02/02 13:40:15 UTC
[GitHub] [nifi] tpalfy opened a new pull request #4798: NIFI-8187 - Add 'Run Once' for processors in context menu.
tpalfy opened a new pull request #4798:
URL: https://github.com/apache/nifi/pull/4798
'Run Once' will start a processor, lets it's `onTrigger` method run one time and then stops it. All lifecylce methods are called as well.
If NiFi is restarted while the processor is running, it will be in 'STOPPED' after restart.
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 edited a comment on pull request #4798: NIFI-8188 - Add 'Run Once' for processors in context menu.
Posted by GitBox <gi...@apache.org>.
markap14 edited a comment on pull request #4798:
URL: https://github.com/apache/nifi/pull/4798#issuecomment-772642426
Thanks @tpalfy, will review
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4798: NIFI-8188 - Add 'Run Once' for processors in context menu.
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4798:
URL: https://github.com/apache/nifi/pull/4798#issuecomment-772642426
Thanks @tpalfy , willy review
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] tpalfy commented on a change in pull request #4798: NIFI-8188 - Add 'Run Once' for processors in context menu.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on a change in pull request #4798:
URL: https://github.com/apache/nifi/pull/4798#discussion_r578700170
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/scheduling/StandardProcessScheduler.java
##########
@@ -352,6 +352,47 @@ public void onTaskComplete() {
return future;
}
+ /**
+ * Runs the given {@link Processor} once by invoking its
+ * {@link ProcessorNode#runOnce(ScheduledExecutorService, long, long, Supplier, SchedulingAgentCallback)}
+ * method.
+ *
+ * @see ProcessorNode#runOnce(ScheduledExecutorService, long, long, Supplier, SchedulingAgentCallback)
+ */
+ @Override
+ public Future<Void> runProcessorOnce(ProcessorNode procNode, final Callable<Future<Void>> stopCallback) {
+ final LifecycleState lifecycleState = getLifecycleState(requireNonNull(procNode), true);
+
+ final Supplier<ProcessContext> processContextFactory = () -> new StandardProcessContext(procNode, getControllerServiceProvider(),
+ this.encryptor, getStateManager(procNode.getIdentifier()), lifecycleState::isTerminated, flowController);
+
+ final CompletableFuture<Void> future = new CompletableFuture<>();
Review comment:
Those three statements each return an object that are required later. So the extracted method would need to have 3 return values. And since they don't really have anything to do with each other, it wouldn't make sense to create a DTO for these arbitrary three types.
Also most of the logic is already extracted into the methods they invoke. These statements themselves contain little to no logic.
I think it's fine to leave them as they are.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4798: NIFI-8188 - Add 'Run Once' for processors in context menu.
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4798:
URL: https://github.com/apache/nifi/pull/4798#issuecomment-772652269
@tpalfy the code here looks good. I'll be testing some corner cases today to see if i can cause any errors. One thing that I noticed, though, is that there is nothing added to the User Guide for this feature. Probably makes sense to update the User Guide to note that the feature exists, and to explain some of the caveats.
For example, if I run in a cluster and I have a processor configured to run on Primary Node only and I click Run Once, it runs only on the Primary Node, not all nodes. If Execution is set to All Nodes, it is triggered once on every node. It's worth clarifying that in the docs.
Also, if the processor's outbound connection has backpressure applied, I am able to click Run Once, but it doesn't actually get triggered. Similarly, if there is an incoming connection and the connection is empty, it doesn't get triggered. These are good things and I believe the correct/desired behavior. But I think it's important to explicitly call these things out in the User Guide.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 closed pull request #4798: NIFI-8188 - Add 'Run Once' for processors in context menu.
Posted by GitBox <gi...@apache.org>.
markap14 closed pull request #4798:
URL: https://github.com/apache/nifi/pull/4798
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] Lehel44 commented on a change in pull request #4798: NIFI-8188 - Add 'Run Once' for processors in context menu.
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on a change in pull request #4798:
URL: https://github.com/apache/nifi/pull/4798#discussion_r572946455
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/scheduling/StandardProcessScheduler.java
##########
@@ -352,6 +352,47 @@ public void onTaskComplete() {
return future;
}
+ /**
+ * Runs the given {@link Processor} once by invoking its
+ * {@link ProcessorNode#runOnce(ScheduledExecutorService, long, long, Supplier, SchedulingAgentCallback)}
+ * method.
+ *
+ * @see ProcessorNode#runOnce(ScheduledExecutorService, long, long, Supplier, SchedulingAgentCallback)
+ */
+ @Override
+ public Future<Void> runProcessorOnce(ProcessorNode procNode, final Callable<Future<Void>> stopCallback) {
+ final LifecycleState lifecycleState = getLifecycleState(requireNonNull(procNode), true);
+
+ final Supplier<ProcessContext> processContextFactory = () -> new StandardProcessContext(procNode, getControllerServiceProvider(),
+ this.encryptor, getStateManager(procNode.getIdentifier()), lifecycleState::isTerminated, flowController);
+
+ final CompletableFuture<Void> future = new CompletableFuture<>();
Review comment:
I think this could be extracted to another method because it is duplicated at 324-329
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4798: NIFI-8188 - Add 'Run Once' for processors in context menu.
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4798:
URL: https://github.com/apache/nifi/pull/4798#issuecomment-772666915
I was able to cause an error with the following scenario. Given, the scenario is contrived, but it can definitely happen "in the wild" based on the timing of events when nodes and joining/rejoining to cluster. Steps to reproduce (assumes a 2 node cluster:
1. Create dataflow: GenerateFlowFile -> DebugFlow -> UpdateAttribute
2. Set DebugFlow's `@OnScheduled Pause Time` property to `1 min`
3. Start the processor
4. Shut down Node 2
5. Remove Node 2 from the cluster
6. Stop processor, wait for thread to complete
7. Click Run Once
8. While processor is running, start Node 2
Node 2 will now join the cluster. However, when the single trigger completes, we will have Node 1 with a Scheduled State of STOPPED, and Node 2 with a Scheduled State of RUNNING. Node 2 did not properly inherit the cluster's Scheduled State for the Processor when the Scheduled State was RUN_ONCE.
I think the issue is because in `StandardFlowSynchronizer` we don't take the `RUN_ONCE` state into account in the `updateProcessGroup` method. On line 918, we have the following block:
```
case STOPPED:
if (procState == ScheduledState.DISABLED) {
procNode.getProcessGroup().enableProcessor(procNode);
} else if (procState == ScheduledState.RUNNING) {
controller.stopProcessor(procNode.getProcessGroupIdentifier(), procNode.getIdentifier());
}
break;
```
I think we need to change this to something like:
```
case STOPPED:
case RUN_ONCE:
if (procState == ScheduledState.DISABLED) {
procNode.getProcessGroup().enableProcessor(procNode);
} else if (procState == ScheduledState.RUNNING || procState == ScheduledState.RUN_ONCE) {
controller.stopProcessor(procNode.getProcessGroupIdentifier(), procNode.getIdentifier());
}
break;
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4798: NIFI-8188 - Add 'Run Once' for processors in context menu.
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4798:
URL: https://github.com/apache/nifi/pull/4798#issuecomment-772693921
Set DebugFlow to use an "onTrigger Pause Time" of "1 min" and then queued up some data and clicked Run Once. The UI immediately shows the processor as stopped with 2 active threads (i.e., 1 active thread on each node in my cluster). This is good. If I then right-click on the Processor, I am given the option to click Terminate. But if I click that, I get an error stating that the Processor is not stopped. I should be able to terminate/interrupt the Run Once action.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on a change in pull request #4798: NIFI-8188 - Add 'Run Once' for processors in context menu.
Posted by GitBox <gi...@apache.org>.
markap14 commented on a change in pull request #4798:
URL: https://github.com/apache/nifi/pull/4798#discussion_r578660742
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/scheduling/StandardProcessScheduler.java
##########
@@ -352,6 +352,47 @@ public void onTaskComplete() {
return future;
}
+ /**
+ * Runs the given {@link Processor} once by invoking its
+ * {@link ProcessorNode#runOnce(ScheduledExecutorService, long, long, Supplier, SchedulingAgentCallback)}
+ * method.
+ *
+ * @see ProcessorNode#runOnce(ScheduledExecutorService, long, long, Supplier, SchedulingAgentCallback)
+ */
+ @Override
+ public Future<Void> runProcessorOnce(ProcessorNode procNode, final Callable<Future<Void>> stopCallback) {
+ final LifecycleState lifecycleState = getLifecycleState(requireNonNull(procNode), true);
+
+ final Supplier<ProcessContext> processContextFactory = () -> new StandardProcessContext(procNode, getControllerServiceProvider(),
+ this.encryptor, getStateManager(procNode.getIdentifier()), lifecycleState::isTerminated, flowController);
+
+ final CompletableFuture<Void> future = new CompletableFuture<>();
Review comment:
The `trigger` method is different in the two. It could perhaps be cleaned up to reduce duplication a bit, but I think it's fine.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4798: NIFI-8188 - Add 'Run Once' for processors in context menu.
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4798:
URL: https://github.com/apache/nifi/pull/4798#issuecomment-772690763
I also found that if you configure DebugFlow with `@OnScheduled Pause Time` set to "80 secs" (or anything over 1 minute) the framework will time it out and interrupt the @OnScheduled task. This is normal (and configurable) and desirable. But in this case, I see that even though I clicked Run Once, it keeps trying to run the processor over and over again. I think in this case, even though the run failed, we need to stop trying to schedule it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4798: NIFI-8188 - Add 'Run Once' for processors in context menu.
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4798:
URL: https://github.com/apache/nifi/pull/4798#issuecomment-781595811
@tpalfy thanks for updating. All looks good to me at this point. Was able to re-run all the same tests that produced issues before and verified that everything worked exactly as expected. Thanks! Merged to `main`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org