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/03/14 18:30:51 UTC
[GitHub] nifi pull request: NIFI-1464 life-cycle refactoring part-2
GitHub user olegz opened a pull request:
https://github.com/apache/nifi/pull/275
NIFI-1464 life-cycle refactoring part-2
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/olegz/nifi NIFI-1464B
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/275.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 #275
----
commit 165cf38709fef65d586ad39792b63f3809a6aed9
Author: Oleg Zhurakousky <ol...@suitcase.io>
Date: 2016-03-14T17:29:41Z
NIFI-1464 life-cycle refactoring part-2
----
---
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 life-cycle refactoring part-2
Posted by olegz <gi...@git.apache.org>.
Github user olegz closed the pull request at:
https://github.com/apache/nifi/pull/275
---
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 life-cycle refactoring part-2
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/275#discussion_r56046482
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/scheduling/AbstractSchedulingAgent.java ---
@@ -33,6 +42,27 @@
*/
abstract class AbstractSchedulingAgent implements SchedulingAgent {
+ protected final FlowEngine flowEngine;
+
+ protected AbstractSchedulingAgent(FlowEngine flowEngine) {
+ this.flowEngine = flowEngine;
+ }
+
+ @Override
+ public <T> T submit(Callable<T> task) throws InterruptedException, TimeoutException, ExecutionException {
+ Future<T> f = flowEngine.submit(task);
+ String timeoutString = NiFiProperties.getInstance().getProperty(NiFiProperties.PROCESSOR_SCHEDULING_TIMEOUT);
+ long onScheduleTimeout = timeoutString == null ? 60000
+ : FormatUtils.getTimeDuration(timeoutString.trim(), TimeUnit.MILLISECONDS);
--- End diff --
I think we should wrap this in a try/catch and catch IllegalArgumentException, which could get thrown by FormatUtils.getTimeDuration if the user makes a typo in the nifi.properties file. If we catch it, we should log a warning, I think, and use the default value of 60,000.
---
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 life-cycle refactoring part-2
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on a diff in the pull request:
https://github.com/apache/nifi/pull/275#discussion_r56073702
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/SchedulingAgentCallback.java ---
@@ -0,0 +1,28 @@
+/*
+ * 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;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.Future;
+
+public interface SchedulingAgentCallback {
--- End diff --
would prefer that these api methods be documented.
---
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 life-cycle refactoring part-2
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/275#discussion_r56044999
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java ---
@@ -1382,21 +1372,16 @@ public void run() {
* be logged (WARN) informing a user so further actions could be taken.
* </p>
*/
- private void invokeTaskAsCancelableFuture(ScheduledExecutorService taskScheduler, Callable<Void> task) {
- Future<Void> executionResult = taskScheduler.submit(task);
-
- String timeoutString = NiFiProperties.getInstance().getProperty(NiFiProperties.PROCESSOR_SCHEDULING_TIMEOUT);
- long onScheduleTimeout = timeoutString == null ? Long.MAX_VALUE
- : FormatUtils.getTimeDuration(timeoutString.trim(), TimeUnit.MILLISECONDS);
-
+ private <T> void invokeTaskAsCancelableFuture(SchedulingAgentCallback callback, Callable<T> task) {
try {
- executionResult.get(onScheduleTimeout, TimeUnit.MILLISECONDS);
+ callback.invokeTaskAsCancelableFuture(task);
} catch (InterruptedException e) {
LOG.warn("Thread was interrupted while waiting for processor '" + this.processor.getClass().getSimpleName()
+ "' lifecycle operation (OnScheduled or OnUnscheduled) to finish.");
Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted while executing one of processor's lifecycle tasks (OnScheduled or OnUnscheduled).", e);
} catch (TimeoutException e) {
- executionResult.cancel(true);
+ //executionResult.cancel(true);
--- End diff --
If we don't need this line, can we remove it, instead of commenting it out?
---
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 life-cycle refactoring part-2
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/275#discussion_r56044931
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java ---
@@ -1314,15 +1312,7 @@ public Void call() throws Exception {
public <T extends ProcessContext & ControllerServiceLookup> void stop(final ScheduledExecutorService scheduler,
final T processContext, final Callable<Boolean> activeThreadMonitorCallback) {
if (this.scheduledState.compareAndSet(ScheduledState.RUNNING, ScheduledState.STOPPING)) { // will ensure that the Processor represented by this node can only be stopped once
- invokeTaskAsCancelableFuture(scheduler, new Callable<Void>() {
- @Override
- public Void call() throws Exception {
- try (final NarCloseable nc = NarCloseable.withNarLoader()) {
- ReflectionUtils.quietlyInvokeMethodsWithAnnotation(OnUnscheduled.class, processor, processContext);
- return null;
- }
- }
- });
+ ReflectionUtils.quietlyInvokeMethodsWithAnnotation(OnUnscheduled.class, processor, processContext);
--- End diff --
This call is delegating to Processor code synchronously. Shouldn't we move this line into the Runnable below, making this the first line of the Runnable? This way, this will happen in the background thread and then the @OnStopped method would be called.
---
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 life-cycle refactoring part-2
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/275#discussion_r56045229
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java ---
@@ -1404,9 +1389,9 @@ private void invokeTaskAsCancelableFuture(ScheduledExecutorService taskScheduler
+ "have been written to ignore interrupts which may result in runaway thread which could lead to more issues "
+ "eventually requiring NiFi to be restarted. This is usually a bug in the target Processor '"
+ this.processor + "' that needs to be documented, reported and eventually fixed.");
+ throw new RuntimeException("Timed out while executing one of processor's lifecycle tasks (OnScheduled or OnUnscheduled).", e);
} catch (ExecutionException e){
- throw new RuntimeException(
- "Failed while executing one of processor's lifecycle tasks (OnScheduled or OnUnscheduled).", e);
+ throw new RuntimeException("Failed while executing one of processor's lifecycle tasks (OnScheduled or OnUnscheduled).", e);
--- End diff --
Can we pass in the name of the lifecycle event here or something? I would avoid indicating "OnScheduled or OnUnscheduled" -- we should know when we generate this message which one it was.
---
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.
---