-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Merged by Bors] - Fix OOM error with manual buffer size specification #380
Conversation
The PR is ready for review, although operator-rs still needs to be updated |
I've started a Test Run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished testing, works fine!
Some points:
- Bump the versions in the test-definition to stackable23.1.0-rc1?
- Error messages could provide better info about what is happening (see other comments) or
- Add checks to reject values that will definitely fail to start up? (e.g. 10Mi as memory)
## Description I have used these functions in my Druid PR already: stackabletech/druid-operator#380 Co-authored-by: Felix Hennig <[email protected]>
Co-authored-by: Malte Sander <[email protected]>
Co-authored-by: Malte Sander <[email protected]>
Co-authored-by: Malte Sander <[email protected]>
Co-authored-by: Malte Sander <[email protected]>
Co-authored-by: Malte Sander <[email protected]>
Co-authored-by: Malte Sander <[email protected]>
Co-authored-by: Malte Sander <[email protected]>
Nice, thx! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors merge |
# Description fixes #359 automatic buffer sizing seems to be the problem in #359 This PR requires a new operator-rs version with the operator-rs features from this PR stackabletech/operator-rs#544 This PR is intended to fix this, by calculating a buffer size based on the user provided memory request. Druid guidelines are used to compute all values. We also set the available `MaxDirectAccessMemory` dynamically, instead of hardcoding it. The heap calculation now incorporates this value, so now it shouldn't happen anymore that the JVM tries to allocate more than there is available to the container. The computation has a few other benefits: - maximum buffer size can now be 2GB instead of 1GB (because we're not using druid `auto` anymore) - a fix memory quantity is reserved for the OS, not a scaled one. This saves memory if more memory is available - there we're bugs that didn't surface yet with the MaxDirectAccessMemory of all roles. All of them are fixed now, the MaxDirectAccessMemory + Heap Memory does not exceed the maximum memory available anymore. - The rounding used for the JVM memory is not unit-dependent anymore. Before it would round of .2 of _whatever_ quantity, so there might be 200MB sitting around unused. This is not the case anymore. - If the direct access memory is maxed out, all the remaining memory will be allocated as Heap, to make maxium use of the allocatable memory. This is also for all roles.
Pull request successfully merged into main. Build succeeded: |
Description
fixes #359
automatic buffer sizing seems to be the problem in #359
This PR requires a new operator-rs version with the operator-rs features from this PR stackabletech/operator-rs#544
This PR is intended to fix this, by calculating a buffer size based on the user provided memory request. Druid guidelines are used to compute all values.
We also set the available
MaxDirectAccessMemory
dynamically, instead of hardcoding it. The heap calculation now incorporates this value, so now it shouldn't happen anymore that the JVM tries to allocate more than there is available to the container.The computation has a few other benefits:
auto
anymore)Review Checklist
Once the review is done, comment
bors r+
(orbors merge
) to merge. Further information