You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by olegz <gi...@git.apache.org> on 2016/02/09 06:33:18 UTC
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
GitHub user olegz opened a pull request:
https://github.com/apache/nifi/pull/210
NIFI-1464, NIFI-78, NIFI-1487 Refactored Processor's life-cycle opera…
…tion sequence
* Simplified and cleaned StandardProcessScheduler.start/stopProcessor methods
* Added stop/start operations to ProcessorNode.
* Removed unnecessary synchronization blocks related to ScheduledState in favor of enforcing order and idempotency via CAS operations. Those synchronization blocks were causing intermittent deadlocks whenever @OnScheduled blocks indefinitely.
* Added support for stopping the service when @OnScheduled operation hangs.
* Fixed the order of life-cycle operation invocation ensuring that each operation can *only* be invoked at the appropriate time
* Removed unnecessary locks from StandardProcessNode since Atomic variables are used.
* Removed calls to @OnStopped from ContinuallyRunningProcessTask while ensuring that procesor's full shut down in implementation of StandardProcessorNode.stop() method.
* Removed dead code
* Added comprehensive tests suite that covers 95% of Processor's life-cycle operations within the scope of FlowController, StandardProcesssScheduler and StandardProcessNode
* Improved and added javadocs on covered operations with detailed explanations.
polishing
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/olegz/nifi NIFI-1464
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/210.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 #210
----
commit aee48bfce95688f8fc0b0851efa75ce91937cff5
Author: Oleg Zhurakousky <ol...@suitcase.io>
Date: 2016-02-04T21:23:45Z
NIFI-1464, NIFI-78, NIFI-1487 Refactored Processor's life-cycle operation sequence
* Simplified and cleaned StandardProcessScheduler.start/stopProcessor methods
* Added stop/start operations to ProcessorNode.
* Removed unnecessary synchronization blocks related to ScheduledState in favor of enforcing order and idempotency via CAS operations. Those synchronization blocks were causing intermittent deadlocks whenever @OnScheduled blocks indefinitely.
* Added support for stopping the service when @OnScheduled operation hangs.
* Fixed the order of life-cycle operation invocation ensuring that each operation can *only* be invoked at the appropriate time
* Removed unnecessary locks from StandardProcessNode since Atomic variables are used.
* Removed calls to @OnStopped from ContinuallyRunningProcessTask while ensuring that procesor's full shut down in implementation of StandardProcessorNode.stop() method.
* Removed dead code
* Added comprehensive tests suite that covers 95% of Processor's life-cycle operations within the scope of FlowController, StandardProcesssScheduler and StandardProcessNode
* Improved and added javadocs on covered operations with detailed explanations.
polishing
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r53043617
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/service/StandardControllerServiceProvider.java ---
@@ -457,6 +457,7 @@ public ControllerServiceNode getControllerServiceNode(final String serviceIdenti
public Set<String> getControllerServiceIdentifiers(final Class<? extends ControllerService> serviceType) {
final Set<String> identifiers = new HashSet<>();
for (final Map.Entry<String, ControllerServiceNode> entry : controllerServices.entrySet()) {
+ Class<? extends ControllerService> c = entry.getValue().getProxiedControllerService().getClass();
--- End diff --
I don't understand why this line was added. Was the intent to reference 'c' below, rather than having that expression inline?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the pull request:
https://github.com/apache/nifi/pull/210#issuecomment-189328683
Otherwise, I think all looks good. I definitely like the simplification of the code. I agree that the locking is no longer necessary. It was there because we wanted to atomically evaluate isRunning() and then update the processor, all as a single unit, without the processor being started in the meantime. However, this is not really a concern, since starting the processor with a separate thread would have to be done via the web tier, which will hold a lock anyway.
Great addition! Once the DTO & FlowSerializer are addressed, and the DISABLED state is added back in, I'll be a +1!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464 Refactored Processor's life-cycle ope...
Posted by olegz <gi...@git.apache.org>.
Github user olegz closed the pull request at:
https://github.com/apache/nifi/pull/210
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the pull request:
https://github.com/apache/nifi/pull/210#issuecomment-189314275
@olegz I noticed in StandardProcessorNode.verifyCanStart, you allowed the Processor to start even if it's DISABLED. I think this is a bug that needs to be addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r53062473
--- Diff: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java ---
@@ -71,6 +71,7 @@
public static final String ADMINISTRATIVE_YIELD_DURATION = "nifi.administrative.yield.duration";
public static final String PERSISTENT_STATE_DIRECTORY = "nifi.persistent.state.directory";
public static final String BORED_YIELD_DURATION = "nifi.bored.yield.duration";
+ public static final String PROCESSOR_START_TIMEOUT = "nifi.processor.start.timeout";
--- End diff --
In looking over this PR more, I'm quite puzzled by this property - it does not appear to be read anywhere either. Is this ever used?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r53165063
--- Diff: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java ---
@@ -71,6 +71,7 @@
public static final String ADMINISTRATIVE_YIELD_DURATION = "nifi.administrative.yield.duration";
public static final String PERSISTENT_STATE_DIRECTORY = "nifi.persistent.state.directory";
public static final String BORED_YIELD_DURATION = "nifi.bored.yield.duration";
+ public static final String PROCESSOR_START_TIMEOUT = "nifi.processor.start.timeout";
--- End diff --
regarding user friendliness the issue we saw before providing a utility for more natural expression of duration is that people were very inconsistent in specifying the unit of time that a given value applied to and using at times periods which didn't make sense for the scenario. This model allows the user to express in a much more natural way both the period of time and unit of time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r52907228
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/scheduling/StandardProcessScheduler.java ---
@@ -288,171 +284,74 @@ public void run() {
}
/**
- * Starts scheduling the given processor to run after invoking all methods on the underlying {@link org.apache.nifi.processor.Processor
- * FlowFileProcessor} that are annotated with the {@link OnScheduled} annotation.
+ * Starts the given {@link Processor} by invoking its
+ * {@link ProcessorNode#start(ScheduledExecutorService, long, org.apache.nifi.processor.ProcessContext, Runnable)}
+ * .
+ * @see StandardProcessorNode#start(ScheduledExecutorService, long,
+ * org.apache.nifi.processor.ProcessContext, Runnable).
*/
@Override
public synchronized void startProcessor(final ProcessorNode procNode) {
- if (procNode.getScheduledState() == ScheduledState.DISABLED) {
--- End diff --
So I am assuming we're cool or should I still explain?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r52926642
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/scheduling/StandardProcessScheduler.java ---
@@ -288,171 +284,74 @@ public void run() {
}
/**
- * Starts scheduling the given processor to run after invoking all methods on the underlying {@link org.apache.nifi.processor.Processor
- * FlowFileProcessor} that are annotated with the {@link OnScheduled} annotation.
+ * Starts the given {@link Processor} by invoking its
+ * {@link ProcessorNode#start(ScheduledExecutorService, long, org.apache.nifi.processor.ProcessContext, Runnable)}
+ * .
+ * @see StandardProcessorNode#start(ScheduledExecutorService, long,
+ * org.apache.nifi.processor.ProcessContext, Runnable).
*/
@Override
public synchronized void startProcessor(final ProcessorNode procNode) {
- if (procNode.getScheduledState() == ScheduledState.DISABLED) {
--- End diff --
Sure, sorry in route to NYC, but will look once back online, but definitely up for it
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r52835796
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/service/TestStandardControllerServiceProvider.java ---
@@ -218,7 +221,7 @@ public void testStartStopReferencingComponents() {
public Object answer(InvocationOnMock invocation) throws Throwable {
final ProcessorNode procNode = (ProcessorNode) invocation.getArguments()[0];
procNode.verifyCanStart();
- procNode.setScheduledState(ScheduledState.RUNNING);
+ // procNode.setScheduledState(ScheduledState.RUNNING);
--- End diff --
Dead code throughout this test class
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r53169078
--- Diff: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java ---
@@ -71,6 +71,7 @@
public static final String ADMINISTRATIVE_YIELD_DURATION = "nifi.administrative.yield.duration";
public static final String PERSISTENT_STATE_DIRECTORY = "nifi.persistent.state.directory";
public static final String BORED_YIELD_DURATION = "nifi.bored.yield.duration";
+ public static final String PROCESSOR_START_TIMEOUT = "nifi.processor.start.timeout";
--- End diff --
One last comment. In any event I think we should still support a simple expression of time in numbers treating them as milliseconds. For example if time configuration value comes as '1', then it should be treated as 1 millisecond. At least this way we can argue both points and see what users use most.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r54588819
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/ProcessorNode.java ---
@@ -99,4 +108,96 @@ public ProcessorNode(final Processor processor, final String id,
*/
public abstract void verifyCanStart(Set<ControllerServiceNode> ignoredReferences);
+ /**
+ *
+ */
+ @Override
+ public ScheduledState getScheduledState() {
+ return this.scheduledState.get();
+ }
+
+ /**
+ * Returns the logical state of this processor. Logical state ignores
+ * transition states such as STOPPING and STARTING rounding it up to the
+ * next logical state of STOPPED and RUNNING respectively.
+ *
+ * @return the logical state of this processor [DISABLED, STOPPED, RUNNING]
+ */
+ public ScheduledState getLogicalScheduledState() {
--- End diff --
Good. I like this approach.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r53167209
--- Diff: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java ---
@@ -71,6 +71,7 @@
public static final String ADMINISTRATIVE_YIELD_DURATION = "nifi.administrative.yield.duration";
public static final String PERSISTENT_STATE_DIRECTORY = "nifi.persistent.state.directory";
public static final String BORED_YIELD_DURATION = "nifi.bored.yield.duration";
+ public static final String PROCESSOR_START_TIMEOUT = "nifi.processor.start.timeout";
--- End diff --
I can see why convention over configuration is strong within a development community but what we observed across a wide variety of a user community was very different need.
Regarding what is arguably 'natural' from one language to another this is a great point that I personally had not considered. Fortunately I believe the set of supported terms to accept as indicators of periods can be substantially expanded though I am not aware of how problematic ambiguity could be. We could of course choose to support some specific standard for expression of time periods though so still have viable paths.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r53057308
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/service/StandardControllerServiceProvider.java ---
@@ -457,6 +457,7 @@ public ControllerServiceNode getControllerServiceNode(final String serviceIdenti
public Set<String> getControllerServiceIdentifiers(final Class<? extends ControllerService> serviceType) {
final Set<String> identifiers = new HashSet<>();
for (final Map.Entry<String, ControllerServiceNode> entry : controllerServices.entrySet()) {
+ Class<? extends ControllerService> c = entry.getValue().getProxiedControllerService().getClass();
--- End diff --
Oops!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r52835777
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/scheduling/StandardProcessScheduler.java ---
@@ -288,171 +284,74 @@ public void run() {
}
/**
- * Starts scheduling the given processor to run after invoking all methods on the underlying {@link org.apache.nifi.processor.Processor
- * FlowFileProcessor} that are annotated with the {@link OnScheduled} annotation.
+ * Starts the given {@link Processor} by invoking its
+ * {@link ProcessorNode#start(ScheduledExecutorService, long, org.apache.nifi.processor.ProcessContext, Runnable)}
+ * .
+ * @see StandardProcessorNode#start(ScheduledExecutorService, long,
+ * org.apache.nifi.processor.ProcessContext, Runnable).
*/
@Override
public synchronized void startProcessor(final ProcessorNode procNode) {
- if (procNode.getScheduledState() == ScheduledState.DISABLED) {
--- End diff --
A bit confused as to where this went. Stepped through the associated implementations but not seeing any place where this is validated.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r53164352
--- Diff: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java ---
@@ -71,6 +71,7 @@
public static final String ADMINISTRATIVE_YIELD_DURATION = "nifi.administrative.yield.duration";
public static final String PERSISTENT_STATE_DIRECTORY = "nifi.persistent.state.directory";
public static final String BORED_YIELD_DURATION = "nifi.bored.yield.duration";
+ public static final String PROCESSOR_START_TIMEOUT = "nifi.processor.start.timeout";
--- End diff --
I don't think we need to add default since it's handled during retrieval (basically if property is null or doesn't exist).
I'll change to string based representation of time although I am still struggling with the whole concept and if indeed it is more user friendly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r52926373
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/scheduling/StandardProcessScheduler.java ---
@@ -288,171 +284,74 @@ public void run() {
}
/**
- * Starts scheduling the given processor to run after invoking all methods on the underlying {@link org.apache.nifi.processor.Processor
- * FlowFileProcessor} that are annotated with the {@link OnScheduled} annotation.
+ * Starts the given {@link Processor} by invoking its
+ * {@link ProcessorNode#start(ScheduledExecutorService, long, org.apache.nifi.processor.ProcessContext, Runnable)}
+ * .
+ * @see StandardProcessorNode#start(ScheduledExecutorService, long,
+ * org.apache.nifi.processor.ProcessContext, Runnable).
*/
@Override
public synchronized void startProcessor(final ProcessorNode procNode) {
- if (procNode.getScheduledState() == ScheduledState.DISABLED) {
--- End diff --
Yeah, we are good here. I mentioned on the backing ticket, but would you also be up for updating the developer's guide with the changes you have employed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the pull request:
https://github.com/apache/nifi/pull/210#issuecomment-189315181
Point taken, wasn't sure what DISABLED meant in the context of Processor and thought it was some leftover that shouldn't be there. Will put it back and add tests for it
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r53038033
--- Diff: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java ---
@@ -71,6 +71,7 @@
public static final String ADMINISTRATIVE_YIELD_DURATION = "nifi.administrative.yield.duration";
public static final String PERSISTENT_STATE_DIRECTORY = "nifi.persistent.state.directory";
public static final String BORED_YIELD_DURATION = "nifi.bored.yield.duration";
+ public static final String PROCESSOR_START_TIMEOUT = "nifi.processor.start.timeout";
--- End diff --
This needs to be added to the nifi.properties file, with a default value added to the assembly's pom.xml, I believe. Also, I believe as-is, it expects a number of milliseconds. Would like to see that changed to using the standard "Time format" that we use in nifi - e.g. "5 secs" instead of "5000"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r52835678
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/ProcessorNode.java ---
@@ -99,4 +100,49 @@ public ProcessorNode(final Processor processor, final String id,
*/
public abstract void verifyCanStart(Set<ControllerServiceNode> ignoredReferences);
+ /**
+ * Will start the {@link Processor} represented by this
+ * {@link ProcessorNode}. Starting processor typically means invoking it's
--- End diff --
minor: it's -> its
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r52835947
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/scheduling/StandardProcessScheduler.java ---
@@ -288,171 +284,74 @@ public void run() {
}
/**
- * Starts scheduling the given processor to run after invoking all methods on the underlying {@link org.apache.nifi.processor.Processor
- * FlowFileProcessor} that are annotated with the {@link OnScheduled} annotation.
+ * Starts the given {@link Processor} by invoking its
+ * {@link ProcessorNode#start(ScheduledExecutorService, long, org.apache.nifi.processor.ProcessContext, Runnable)}
+ * .
+ * @see StandardProcessorNode#start(ScheduledExecutorService, long,
+ * org.apache.nifi.processor.ProcessContext, Runnable).
*/
@Override
public synchronized void startProcessor(final ProcessorNode procNode) {
- if (procNode.getScheduledState() == ScheduledState.DISABLED) {
--- End diff --
Drat, the GitHub view foiled me while searching around.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r53169197
--- Diff: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java ---
@@ -71,6 +71,7 @@
public static final String ADMINISTRATIVE_YIELD_DURATION = "nifi.administrative.yield.duration";
public static final String PERSISTENT_STATE_DIRECTORY = "nifi.persistent.state.directory";
public static final String BORED_YIELD_DURATION = "nifi.bored.yield.duration";
+ public static final String PROCESSOR_START_TIMEOUT = "nifi.processor.start.timeout";
--- End diff --
I am definitely on the same page with you there https://issues.apache.org/jira/browse/NIFI-1522
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r52926955
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/scheduling/StandardProcessScheduler.java ---
@@ -288,171 +284,74 @@ public void run() {
}
/**
- * Starts scheduling the given processor to run after invoking all methods on the underlying {@link org.apache.nifi.processor.Processor
- * FlowFileProcessor} that are annotated with the {@link OnScheduled} annotation.
+ * Starts the given {@link Processor} by invoking its
+ * {@link ProcessorNode#start(ScheduledExecutorService, long, org.apache.nifi.processor.ProcessContext, Runnable)}
+ * .
+ * @see StandardProcessorNode#start(ScheduledExecutorService, long,
+ * org.apache.nifi.processor.ProcessContext, Runnable).
*/
@Override
public synchronized void startProcessor(final ProcessorNode procNode) {
- if (procNode.getScheduledState() == ScheduledState.DISABLED) {
--- End diff --
No worries and no rush. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the pull request:
https://github.com/apache/nifi/pull/210#issuecomment-189326513
In StandardFlowSerializer, Line 314, we write out the Processor's Scheduled State. This is done so that when we restore the flow on restart, we know what state the Processor is in. So if we are adding new states to Scheduled State, we need to be sure to account for that there. Specifically, I think we should just update StandardFlowSerializer to write out a state of RUNNING if the state is STARTING and a state of STOPPED if it's STOPPING. Then we don't have to worry about the code that restores the state because what is written to the flow.xml is the state that we want to use when we restore the flow.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r53166575
--- Diff: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java ---
@@ -71,6 +71,7 @@
public static final String ADMINISTRATIVE_YIELD_DURATION = "nifi.administrative.yield.duration";
public static final String PERSISTENT_STATE_DIRECTORY = "nifi.persistent.state.directory";
public static final String BORED_YIELD_DURATION = "nifi.bored.yield.duration";
+ public static final String PROCESSOR_START_TIMEOUT = "nifi.processor.start.timeout";
--- End diff --
Very true and unfortunately this is where I would go back to the "convention-over-configuration" discussion where if we declare that MILLISECONDS is a convention (the most widely used, hence natural and intuitive) for expression of all things time related then consistency would come.
I am just afraid that we are actually asking user to do more (think more), rather then less. On top of that it creates more possibilities of spelling errors to deal with in the event user describes it in a way we can't parse, which may come with international users who I presume may naturally type things in their own language variation ('seconde', 'segundo' etc.)
Anyway, I've changed it, so we're good here, but would like this discussion to continue at some point may be on the WIKI or dev list.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r52835679
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/scheduling/AbstractSchedulingAgent.java ---
@@ -0,0 +1,103 @@
+/*
+ * 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.controller.scheduling;
+
+import org.apache.nifi.connectable.Connectable;
+import org.apache.nifi.controller.ReportingTaskNode;
+
+/**
+ * Base implementation of the {@link SchedulingAgent} which encapsulates the
+ * updates to the {@link ScheduleState} based on invoked operation and then
+ * delegates to the corresponding 'do' methods. For example; By invoking
+ * {@link #schedule(Connectable, ScheduleState)} the the
--- End diff --
minor: duplicate the
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the pull request:
https://github.com/apache/nifi/pull/210#issuecomment-189327203
We also have the same above issue with the ProcessorDTO - in DtoFactory.createProcessorDto, we need to set the state to RUNNING or STOPPED - STARTING and STOPPING are not valid states for the DTO.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1464, NIFI-78, NIFI-1487 Refactored Proces...
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on a diff in the pull request:
https://github.com/apache/nifi/pull/210#discussion_r52907459
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/service/TestStandardControllerServiceProvider.java ---
@@ -218,7 +221,7 @@ public void testStartStopReferencingComponents() {
public Object answer(InvocationOnMock invocation) throws Throwable {
final ProcessorNode procNode = (ProcessorNode) invocation.getArguments()[0];
procNode.verifyCanStart();
- procNode.setScheduledState(ScheduledState.RUNNING);
+ // procNode.setScheduledState(ScheduledState.RUNNING);
--- End diff --
True, and I think this test(s) may need to be removed since tests that I added as part of the refactoring efforts cover all possible scenarios without Mocks. And if for some reason you'll find a missing scenario, let me know and I'll add a test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---