markap14 commented on code in PR #7242:
URL: https://github.com/apache/nifi/pull/7242#discussion_r1202933799
##########
nifi-stateless/nifi-stateless-api/src/main/java/org/apache/nifi/stateless/config/PropertiesFileEngineConfigurationParser.java:
##########
@@ -53,6 +54,9 @@ public class PropertiesFileEngineConfigurationParser {
private static final String CONTENT_REPO_DIRECTORY = PREFIX +
"content.repository.directory";
private static final String STATUS_TASK_INTERVAL = PREFIX +
"status.task.interval";
+ private static final String COMPONENT_ENABLE_TIMEOUT = PREFIX +
"component.enableTimeout";
+ private static final String PROCESSOR_START_TIMEOUT = PREFIX +
"processor.startTimeout";
Review Comment:
We should use dotted names instead of camel-case -
`component.enable.timeout` and `processor.start.timeout` rather than
`enableTimeout` and `startTimeout`
##########
nifi-stateless/nifi-stateless-bundle/nifi-stateless-engine/src/main/java/org/apache/nifi/stateless/flow/StatelessFlowConfiguration.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * 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.stateless.flow;
+
+public class StatelessFlowConfiguration {
Review Comment:
I don't think this class is necessary. It's simply a wrapper around a long.
But a `StatelessFlowConfiguration` would imply a great deal more configuration.
We should instead just provide the timeout to the Stateless Flow
##########
nifi-stateless/nifi-stateless-api/src/main/java/org/apache/nifi/stateless/engine/StatelessEngineConfiguration.java:
##########
@@ -89,4 +90,20 @@ default boolean isLogExtensionDiscovery() {
* A <code>null</code> value indicates that no status tasks are scheduled.
*/
String getStatusTaskInterval();
+
+ /**
+ * @return a String representing the number of milliseconds that the
process scheduler should wait for a process to start
+ * Defaults to 10 seconds or 10_000 milliseconds
+ */
+ default long getProcessorStartTimeout() {
Review Comment:
Should take an argument for the `TimeUnit` to use
##########
nifi-stateless/nifi-stateless-bundle/nifi-stateless-engine/src/main/java/org/apache/nifi/stateless/engine/StandardStatelessEngine.java:
##########
@@ -658,6 +662,10 @@ public CounterRepository getCounterRepository() {
public Duration getStatusTaskInterval() {
return statusTaskInterval;
}
+ @Override
+ public StatelessEngineConfiguration getStatelessEngineConfiguration() {
+ return statelessEngineConfiguration;
+ }
Review Comment:
Method is unnecessary and can be removed.
##########
nifi-stateless/nifi-stateless-bundle/nifi-stateless-engine/src/main/java/org/apache/nifi/stateless/engine/StandardStatelessEngine.java:
##########
@@ -733,6 +742,11 @@ public Builder statusTaskInterval(final String
statusTaskInterval) {
return this;
}
+ public Builder statelessEngineConfiguration(final
StatelessEngineConfiguration statelessEngineConfiguration) {
+ this.statelessEngineConfiguration = statelessEngineConfiguration;
+ return this;
+ }
Review Comment:
The only thing that we need to pass in here is the timeout - we should avoid
passing in the entire StatelessEngineConfiguration and just take in the timeout
##########
nifi-stateless/nifi-stateless-api/src/main/java/org/apache/nifi/stateless/engine/StatelessEngineConfiguration.java:
##########
@@ -89,4 +90,20 @@ default boolean isLogExtensionDiscovery() {
* A <code>null</code> value indicates that no status tasks are scheduled.
*/
String getStatusTaskInterval();
+
+ /**
+ * @return a String representing the number of milliseconds that the
process scheduler should wait for a process to start
+ * Defaults to 10 seconds or 10_000 milliseconds
+ */
+ default long getProcessorStartTimeout() {
+ return TimeUnit.SECONDS.toMillis(10);
+ }
+
+ /**
+ * @return a String representing the number of milliseconds that the
StatelessEngine should wait for a component to enable
+ * Defaults to 10 seconds or 10_000 milliseconds
+ */
+ default long getComponentEnableTimeout() {
Review Comment:
Should take an argument for the `TimeUnit` to use
##########
nifi-stateless/nifi-stateless-api/src/main/java/org/apache/nifi/stateless/config/PropertiesFileEngineConfigurationParser.java:
##########
@@ -161,6 +168,16 @@ public List<ExtensionClientDefinition>
getExtensionClients() {
public String getStatusTaskInterval() {
return statusTaskInterval;
}
+
+ @Override
+ public long getProcessorStartTimeout() {
Review Comment:
Should take an argument for the `TimeUnit` to use
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/ProcessSchedulerConfig.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * 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;
+
+public class ProcessSchedulerConfig {
Review Comment:
Not sure that this class really provides any benefit over providing the
timeout directly to the scheduler.
##########
nifi-stateless/nifi-stateless-api/src/main/java/org/apache/nifi/stateless/config/PropertiesFileEngineConfigurationParser.java:
##########
@@ -111,6 +115,9 @@ public StatelessEngineConfiguration
parseEngineConfiguration(final File properti
final String statusTaskInterval =
properties.getProperty(STATUS_TASK_INTERVAL, "1 min");
+ final long processorStartTimeout =
TimeUnit.SECONDS.toMillis(Long.parseLong(properties.getProperty(PROCESSOR_START_TIMEOUT,
"10")));
+ final long componentEnableTimeout =
TimeUnit.SECONDS.toMillis(Long.parseLong(properties.getProperty(COMPONENT_ENABLE_TIMEOUT,
"10")));
Review Comment:
Timeout properties should never be integer values - they need to always be
time periods. So "10 secs" is a reasonable default, but we need to parse the
property value using `FormatUtils.getTimeDuration`
##########
nifi-stateless/nifi-stateless-bundle/nifi-stateless-engine/src/main/java/org/apache/nifi/stateless/engine/StatelessEngine.java:
##########
@@ -71,4 +71,6 @@ public interface StatelessEngine {
CounterRepository getCounterRepository();
Duration getStatusTaskInterval();
+
+ StatelessEngineConfiguration getStatelessEngineConfiguration();
Review Comment:
This is unnecessary. It's not used anywhere.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
