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-13865: Enable the JVM to record a heap dump when an OOM error occurs #9380

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mattyb149
Copy link
Contributor

Summary

NIFI-13865. This PR adds the following arguments to bootstrap.conf for the NiFi JVM to enable taking a heap dump when an OutOfMemoryError occurs. I'm not sure about the feasibility of unit/integration testing

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

@joewitt
Copy link
Contributor

joewitt commented Oct 11, 2024

This needs testing in real world deployments before it becomes an always on default. We've used this many times in the past but there was always some reason to avoid it being there as a default.

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 proposing this change @mattyb149.

Although enabling heap dumps can be very useful for troubleshooting, it can also present security concerns due to the memory contents containing sensitive values. For this reason, it is not something that we should enable as part of the default configuration.

@mattyb149
Copy link
Contributor Author

Makes sense, I'll add them commented out with some doc

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.

Instead of adding this to the configuration file, it would be better to note this as an option in the Administrator's Guide. The security implications are worth highlighting, and although it requires going to the guide, this is also something that should not necessarily be used regularly.

@joewitt
Copy link
Contributor

joewitt commented Oct 11, 2024

good idea @exceptionfactory. In general getting our out of the box files simpler/less options and then documenting how to do things would yield a better user experience.

@mattyb149
Copy link
Contributor Author

I can add it to the Admin Guide too, but for folks supporting other NiFi users, it just seems easier to ask the user(s) to uncomment a couple of existing lines vs having them search in the Guide for something that they might not know they're looking for, mis-copying the text from the Guide, etc.

@exceptionfactory
Copy link
Contributor

I can add it to the Admin Guide too, but for folks supporting other NiFi users, it just seems easier to ask the user(s) to uncomment a couple of existing lines vs having them search in the Guide for something that they might not know they're looking for, mis-copying the text from the Guide, etc.

I agree this is a fine line, but over the years, the default configuration files have grown to include lot of commented information and many defaults that rarely change. In this case, the security implications and potential disk utilization concerns surrounding heap dumps make me think this should not be included in the default bootstrap.conf, even as a commented option.

@exceptionfactory
Copy link
Contributor

It is also worth noting that the while the default configuration files should be minimal, nothing prevents others from providing alternative defaults in customized packages. The goal of the default files, however, should be to provide sensible and secure defaults without overwhelming users.

@mattyb149
Copy link
Contributor Author

I was using the debug and javaAgent entries as an example.

@exceptionfactory
Copy link
Contributor

I agree the debug and javaAgent examples are notable. There are other common production-level options we could consider, such as maximum directory memory usage, and other memory settings, but these are more specific to given environments, and harder to generalize. That's where having a section of the Admin Guide, with copyable examples, seems like a better way to go more for more JVM options.

@mattyb149 mattyb149 added the hacktoberfest-accepted Hacktoberfest Accepted label Oct 18, 2024
@mattyb149
Copy link
Contributor Author

Ok I will change this PR/Jira to add this to the Admin Guide instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Hacktoberfest Accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants