NIFI-11471: Define new stateless configuration points by ohnoitsyou · Pull Request #7242 · apache/nifi

ohnoitsyou · 2023-05-12T15:52:34Z

NIFI-11471:
Add two new properties:

nifi.stateless.component.enableTimeout
nifi.stateless.processor.startTimeout

to allow configuring the StatelessEngine and ProcessScheduler.

This allows an operator to configure what kind of startup time the flow can tolerate.

Previously these values were hard coded.

Summary

NIFI-11471

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Apache NiFi Jira issue created

Pull Request Tracking

Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

Pull Request based on current revision of the main branch
Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

Build completed using mvn clean install -P contrib-check
- JDK 11
- JDK 17

Licensing

New dependencies are compatible with the Apache License 2.0 according to the License Policy
New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

Documentation formatting appears as expected in rendered files

exceptionfactory · 2023-05-12T18:12:06Z

Thanks for the contribution @ohnoitsyou. All of the automated checks failed, so please run the local contrib-check profile with a full build to ensure all existing tests pass. As a framework-level change, this will require some careful review.

ohnoitsyou · 2023-05-16T19:06:27Z

I think I've got it figured out, just dealing with the intermittent test failures

markap14

Thanks for the update @ohnoitsyou. I do think that it introduces a good bit of complexity that is unnecessary, though, and does not adhere to standard practices such as using time periods like 10 secs for properties. See comments inline. Thanks!

.../src/main/java/org/apache/nifi/stateless/config/PropertiesFileEngineConfigurationParser.java

markap14 · 2023-05-23T20:05:27Z

.../src/main/java/org/apache/nifi/stateless/config/PropertiesFileEngineConfigurationParser.java

+        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")));


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

@markap14 After a quick chat with David H. I brought up that FormatUtils is located within nifi-utils which is something stateless doesn't already import. What would be your opinion on including this? If the answer is "don't", how would you recommend converting from strings to Duration?

So it's okay to bring nifi-utils into nifi-stateless-engine. Definitely not ok to bring it into nifi-stateless-api though. So we should probably have the StatelessEngineConfiguration return a String representation instead of a Duration. Then, do the conversion in the nifi-stateless-engine module where it'll be used.

It probably actually makes sense to move the FormatUtils functionality into nifi-api because there have been other places where it'd be nice to have it available, such as directly in NiFiProperties. But that's beyond the scope of this Jira :) But something to consider for NiFi 2.0

Thanks for the summary @markap14, the FormatUtils has some deprecated methods that need to be removed for NiFi 2.0, and I could see value in pulling it out of nifi-utils and into nifi-api. In the meantime, it sounds like having the Configuration method return a String value and doing the conversion elsewhere is the best option.

markap14 · 2023-05-23T20:05:59Z

.../src/main/java/org/apache/nifi/stateless/config/PropertiesFileEngineConfigurationParser.java

            }
+
+            @Override
+            public long getProcessorStartTimeout() {


Should take an argument for the TimeUnit to use

markap14 · 2023-05-23T20:06:59Z

...ateless-api/src/main/java/org/apache/nifi/stateless/engine/StatelessEngineConfiguration.java

+     * @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() {


Should take an argument for the TimeUnit to use

Using java.time.Duration could provide another more flexible approach that avoids confusion related to time units.

I would definitely be okay with that as well @exceptionfactory

markap14 · 2023-05-23T20:07:12Z

...ateless-api/src/main/java/org/apache/nifi/stateless/engine/StatelessEngineConfiguration.java

+     * @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() {


Should take an argument for the TimeUnit to use

markap14 · 2023-05-23T20:08:53Z

...stateless-engine/src/main/java/org/apache/nifi/stateless/engine/StandardStatelessEngine.java

+        public Builder statelessEngineConfiguration(final StatelessEngineConfiguration statelessEngineConfiguration) {
+            this.statelessEngineConfiguration = statelessEngineConfiguration;
+            return this;
+        }


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

markap14 · 2023-05-23T20:15:19Z

...tateless-engine/src/main/java/org/apache/nifi/stateless/flow/StatelessFlowConfiguration.java

+
+package org.apache.nifi.stateless.flow;
+
+public class StatelessFlowConfiguration {


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

To justify this and for me to understand the future development strategy, if there were more properties that would/could be configurable, if not this pattern, how would you suggest passing these additional config points?

Additional config elements can be passed in the constructor as well. Or we could use a Builder. Typically I'd recommend a Builder pattern after about 4-5 constructor args. We could also potentially provide a Configuration type of class like this in the future, if necessary. If this were a public API I think a "config" type of property might make more sense - but we would either want to rename it, or truly encapsulate all of the configuration elements for a Stateless Flow in it. But in this case, it's not a public API, so we can change it any time.

markap14 · 2023-05-23T20:16:22Z

...le/nifi-stateless-engine/src/main/java/org/apache/nifi/stateless/engine/StatelessEngine.java


    Duration getStatusTaskInterval();
+
+    StatelessEngineConfiguration getStatelessEngineConfiguration();


This is unnecessary. It's not used anywhere.

markap14 · 2023-05-23T20:17:00Z

...stateless-engine/src/main/java/org/apache/nifi/stateless/engine/StandardStatelessEngine.java

+    @Override
+    public StatelessEngineConfiguration getStatelessEngineConfiguration() {
+        return statelessEngineConfiguration;
+    }


Method is unnecessary and can be removed.

markap14 · 2023-05-23T20:17:55Z

...nifi-framework-core-api/src/main/java/org/apache/nifi/controller/ProcessSchedulerConfig.java

+
+package org.apache.nifi.controller;
+
+public class ProcessSchedulerConfig {


Not sure that this class really provides any benefit over providing the timeout directly to the scheduler.

To justify this and for me to understand the future development strategy, if there were more properties that would/could be configurable, if not this pattern, how would you suggest passing these additional config points?

See comment above.

Add two new properties: nifi.stateless.component.enableTimeout nifi.stateless.processor.startTimeout to allow configuring the StatelessEngine and ProcessScheduler. This allows an operator to configure what kind of startup time the flow can tolerate. Previously these values were hard coded.

Expanding out a getter to 3 lines Removing an extra import

…eTimeout StatelessEngineConfiguration is used in several places and in these additional contexts getProcessorStartTimeout and getComponentEnableTimeout don't really mean anything so adding a default implementation works.

…tainers Directly provide new config parameters to the relevant places instead of wrapping them in extra config containers. At the current level of complexity, they are unnecessary. The builder pattern would be more appropriate and inline with current development. Store the parameter values as strings and convert them to `Duration` values and then finally to the appropriate time scale when required.

ohnoitsyou · 2023-06-01T19:51:01Z

@markap14 @exceptionfactory I have rebased this branch on the current main and made what I believe are the requested fixes. It appears to build on my end. If you could approve the workflow, I think it'll build successfully here in CI.

LMK if you think I've captured what you've asked for.

…imeout. This closes #7242.

markap14 · 2023-06-12T18:52:46Z

Thanks @ohnoitsyou changes look good to me. Was able to verify behavior and added and additional system test to verify. +1 merged to both main and support/nifi-1.x branches.

ohnoitsyou force-pushed the NIFI-11471 branch from e4b8025 to 5633b95 Compare May 12, 2023 16:17

ohnoitsyou force-pushed the NIFI-11471 branch from ccb9816 to ae9f60e Compare May 15, 2023 19:03

markap14 requested changes May 23, 2023

View reviewed changes

ohnoitsyou added 4 commits June 1, 2023 19:47

NIFI-11471: Checkstyle violations

0785054

Expanding out a getter to 3 lines Removing an extra import

ohnoitsyou force-pushed the NIFI-11471 branch from ae9f60e to d09817f Compare June 1, 2023 19:47

markap14 closed this in 15f8f87 Jun 12, 2023

markap14 added a commit that referenced this pull request Jun 12, 2023

NIFI-11471: Added a system test to verify controller service enable t…

125e456

…imeout. This closes #7242.

ohnoitsyou deleted the NIFI-11471 branch June 12, 2023 19:44

		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")));


		package org.apache.nifi.stateless.flow;

		public class StatelessFlowConfiguration {


		Duration getStatusTaskInterval();

		StatelessEngineConfiguration getStatelessEngineConfiguration();


		package org.apache.nifi.controller;

		public class ProcessSchedulerConfig {

Conversation

ohnoitsyou commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

Uh oh!

exceptionfactory commented May 12, 2023

Uh oh!

ohnoitsyou commented May 16, 2023

Uh oh!

markap14 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ohnoitsyou commented Jun 1, 2023

Uh oh!

markap14 commented Jun 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ohnoitsyou commented May 12, 2023 •

edited

Loading