Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NIFI-12335 Fix MiNiFi startup issue #7998

Closed
wants to merge 2 commits into from
Closed

NIFI-12335 Fix MiNiFi startup issue #7998

wants to merge 2 commits into from

Conversation

ferencerdei
Copy link
Contributor

@ferencerdei ferencerdei commented Nov 8, 2023 •
edited
Loading

Summary

NIFI-12335

Tracking

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

Issue Tracking

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 21

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

@ferencerdei ferencerdei added the minifi Pull requests that updates minifi/c2 codes label Nov 8, 2023
bejancsaba
bejancsaba reviewed Nov 8, 2023
View reviewed changes
Copy link
Contributor

@bejancsaba bejancsaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks for the change I had 2 quick questions.

...ommons-framework/src/main/java/org/apache/nifi/minifi/commons/service/FlowEnrichService.java
@@ -90,6 +91,9 @@ public byte[] enrichFlow(byte[] flowCandidate) {
}

VersionedDataflow versionedDataflow = parseVersionedDataflow(flowCandidate);
versionedDataflow.setReportingTasks(ofNullable(versionedDataflow.getReportingTasks()).orElseGet(ArrayList::new));
versionedDataflow.setRegistries(ofNullable(versionedDataflow.getRegistries()).orElseGet(ArrayList::new));
versionedDataflow.setControllerServices(ofNullable(versionedDataflow.getControllerServices()).orElseGet(ArrayList::new));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do this for the others (e.g.: parameters) as well or only these are needed to be initialised on case of an empty flow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For ParameterProviders and FlowAnalysisRules it is handled in the VersionedFlowSynchronizer

...ommons-framework/src/main/java/org/apache/nifi/minifi/commons/service/FlowEnrichService.java Outdated
@@ -103,10 +107,12 @@ public byte[] enrichFlow(byte[] flowCandidate) {
rootGroup.setInstanceIdentifier(randomUUID().toString());
}

rootGroup.getControllerServices().forEach(cs -> cs.setScheduledState(ENABLED));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated to the startup issue, is this specific to NiFi 2.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also caused issues because the controller services were not enabled automatically so I got validation errors. I forgot to mention this in the ticket. Because it makes no sense to disable separate components in the context of MiNiFi, I think it's safe to always enable everything.

briansolo1985
briansolo1985 reviewed Nov 8, 2023
View reviewed changes
Copy link
Contributor

@briansolo1985 briansolo1985 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Ferenc, thank you for this change. The approach looks good, and will make this part of the code more fault tolerant.
Wrote some comments please take a look when you'll have the time.

...ommons-framework/src/main/java/org/apache/nifi/minifi/commons/service/FlowEnrichService.java Outdated
@@ -103,10 +107,12 @@ public byte[] enrichFlow(byte[] flowCandidate) {
rootGroup.setInstanceIdentifier(randomUUID().toString());
}

rootGroup.getControllerServices().forEach(cs -> cs.setScheduledState(ENABLED));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor, but in my opinion lamdbas are more expresseive if meaningiful parameter names are used, in this case I would use controllerService, but leave it you.

...ommons-framework/src/main/java/org/apache/nifi/minifi/commons/service/FlowEnrichService.java Outdated
Optional<VersionedControllerService> commonSslControllerService = createCommonSslControllerService();
commonSslControllerService
.ifPresent(sslControllerService -> {
List<VersionedControllerService> currentControllerServices = ofNullable(versionedDataflow.getControllerServices()).orElseGet(ArrayList::new);
List<VersionedControllerService> currentControllerServices = new ArrayList<>(versionedDataflow.getControllerServices());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the initialization was moved outside now it is guaranteed that we will get a non-null list object here.
Is it necessary to recreate the list object for some reasons?
If not we could simplify the ifPresent block by reducing it to

commonSslControllerService.ifPresent(versionedDataflow.getControllerServices()::add);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid passing the reference for the original list, but maybe you are right. As this object doesn't exist outside of this class we can safely use the value directly. I'm going to update this and the other as well.

...ommons-framework/src/main/java/org/apache/nifi/minifi/commons/service/FlowEnrichService.java Outdated
@@ -118,7 +124,7 @@ public byte[] enrichFlow(byte[] flowCandidate) {

createProvenanceReportingTask(commonSslControllerService.map(VersionedComponent::getInstanceIdentifier).orElse(EMPTY))
.ifPresent(provenanceReportingTask -> {
List<VersionedReportingTask> currentReportingTasks = ofNullable(versionedDataflow.getReportingTasks()).orElseGet(ArrayList::new);
List<VersionedReportingTask> currentReportingTasks = new ArrayList<>(versionedDataflow.getReportingTasks());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this one also could be simplified

createProvenanceReportingTask(commonSslControllerService.map(VersionedComponent::getInstanceIdentifier).orElse(EMPTY))
            .ifPresent(versionedDataflow.getReportingTasks()::add);

...ommons-framework/src/main/java/org/apache/nifi/minifi/commons/service/FlowEnrichService.java
@@ -26,6 +26,7 @@
import static java.util.UUID.randomUUID;
import static java.util.stream.Collectors.toMap;
import static org.apache.commons.lang3.StringUtils.EMPTY;
import static org.apache.nifi.flow.ScheduledState.ENABLED;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use the static import in line 159 as well?

@bejancsaba
Copy link
Contributor

Thanks for the details and the changes will merge as soon as the workflows conclude. +1 from my side

@bejancsaba bejancsaba closed this in 4430eff Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bejancsaba bejancsaba bejancsaba left review comments

@briansolo1985 briansolo1985 briansolo1985 left review comments

Assignees
No one assigned
Labels
minifi Pull requests that updates minifi/c2 codes
Projects
None yet
Milestone
No milestone
3 participants
@ferencerdei @bejancsaba @briansolo1985