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.

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

NIFI-12506 /nifi-api/flow/metrics endpoint times out if flow is big #8158

Closed
wants to merge 6 commits into from

Conversation

timeabarna
Copy link
Contributor

@timeabarna timeabarna commented Dec 14, 2023

Summary

If a flow is relatively big, having more than 10K processors, /nifi-api/flow/metrics endpoint times out if nifi.analytics.predict.enabled set to true as gathering predictions takes long.
This PR modify the prediction gathering to run in parallel to speed up the processing.

NIFI-12506

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

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for working on this issue @timeabarna.

One general note, creating a thread pool for each method invocation can be expensive, and is not ideal from a resource reuse perspective. This may be a good opportunity to use virtual threads, as opposed to creating another long-lived thread pool. One of those options should be considered.

@timeabarna
Copy link
Contributor Author

Thanks @markap14 and @exceptionfactory for your review. I've removed the new properties and modified the code to reuse the thread pool as this functionality is heavily used with the 1.x version where Virtual Threads not available.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for making the adjustments @timeabarna. Creating the thread pool once is helpful, but it also highlights a concern about whether this is necessary when analytic prediction is disabled. That's the value of the virtual-thread approach, but I understand the concern for a solution that also works on NiFi 1.

This seems like a case where it would be best to implement this pull request for NiFi 2 with virtual threads, and then submit a second pull request for the support branch. The support branch could optionally create the thread pool. Theoretically the same approach could be taken in this pull request, but even making thread pool creation conditional on the analytics prediction is not as clean as virtual threads.

@timeabarna
Copy link
Contributor Author

Thanks @exceptionfactory for your review. I've updated this PR with Virtual Threads and opened this PR for the support branch.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for making the adjustments @timeabarna, unfortunately the approach of initializing the executor in the method is not thread-safe, so it should be initialized when the NiFiProperties are first provided to the class.

@timeabarna
Copy link
Contributor Author

Thanks @exceptionfactory I've updated both pull requests based on your recommendation.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for the adjustment @timeabarna. I should have mentioned this previously, but creating an ExecutorService requires another lifecycle operation to close the service on shutdown.

Taking step back from this, it looks like a cleaner solution, in light of the lifecycle concern, would be to define the ExecutorService outside of the StandardNiFiServiceFacade itself, and instead inject in the Spring context configuration that provides other services. That avoids the new PostConstruct method, and allows Spring to use standard lifecycle handling to manage the ExecutorService. Unfortunately this means another change is necessary, but this will make the approach more robust when starting and stopping NiFi.

@timeabarna
Copy link
Contributor Author

Thanks @exceptionfactory , I've created a processing service and updated both PRs.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for making the adjustments @timeabarna, moving the processing to a separate service looks like a reasonable approach.

The VirtualThreadParallelProcessingService still needs to close the ExecutorService that gets created in the constructor. If Processing Service implements the Closeable interface and calls ExecutorService.close() in that method, then Spring will shutdown the service cleanly. That should be the last change required.

@timeabarna
Copy link
Contributor Author

Thanks @exceptionfactory, I've updated both PRs with shutdown.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for working through the feedback @timeabarna, the latest version looks good and manages the executor lifecycle as expected. +1 merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants