Skip to content

Conversation

ashangit
Copy link
Contributor

@ashangit ashangit commented Jun 3, 2024

What is the purpose of the change

If the heap usage with overhead leads to remaining memory to be assigned to the HEAP no memory will be assigned to the METASPACE leading to JVM failure at startup
By setting this before the HEAP we ensure the JVM can start with appropriate amount of METASPACE memory

Brief change log

(for example:)

  • Compute METASPACE size to set before the HEAP size

Verifying this change

This change added tests and can be verified as follows:

  • Extended tests to validate that despite a big HEAP usage that would lead to the whole memory being used by it, the METASPACE has been well set with all appropriate memory

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changes to the CustomResourceDescriptors: no
  • Core observer or reconciler logic that is regularly executed: yes

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not documented

@1996fanrui 1996fanrui self-assigned this Jun 5, 2024
Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

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

Thanks @ashangit for the detailed analysis and fix!

LGTM, and I hope to wait for @mxm 's double check before merging.

Copy link
Contributor

@mateczagany mateczagany left a comment

Choose a reason for hiding this comment

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

I think this makes sense and a good way to fix the original issue.

LGTM

If the heap usage with overhead leads to remaining memory to be assigned to the HEAP no
memory will be assigned to the METASPACE leading to JVM failure at startup
By setting this before the HEAP we ensure the JVM can start with appropriate amount of METASPACE memory
@ashangit ashangit force-pushed the nfraison/FLINK-35489 branch from 4e4eff9 to a2bc90b Compare June 10, 2024 12:47
@ashangit
Copy link
Contributor Author

Updated the PR to take in account this comment in the jira ticket

Copy link
Contributor

@mxm mxm left a comment

Choose a reason for hiding this comment

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

Good fix @ashangit!

@1996fanrui
Copy link
Member

Thanks @ashangit for the fix and everyone for the review!

Merging~

@1996fanrui 1996fanrui merged commit c3e94ae into apache:main Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants