Add min/max size methods for MemorySizeSetting #19230
Conversation
Signed-off-by: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
| public static Setting<ByteSizeValue> memorySizeSetting( | ||
| String key, | ||
| String defaultPercentage, | ||
| String minPercentage, |
There was a problem hiding this comment.
I am wondering if there are really any real use cases of specifying the minPercentage for memorySizeSetting. Why would be make something use minimum memory, the objective is mostly limiting the maximum amount of consumed memory by a component. Probably, that is the reason we never required explicit min/max and the default value kind of served as the maximum limit
There was a problem hiding this comment.
I'd be ok with just having it be maximum. However while checking I did find one setting BlobStoreRepository.SNAPSHOT_REPOSITORY_DATA_CACHE_THRESHOLD (link) where they shove the parser logic into a lambda in the setting definition themselves, so that they can define a nonzero minimum of 500b, and a maximum of 0.01% of heap, so I guess there is at least some use case for a minimum memory setting.
There was a problem hiding this comment.
That still seems like arbitrary low value to me, not minimum threshold that user really needs to set. Maybe we can assume nonzero minimum of 500b as the hardcoded minimum for memorySizeSetting. Are you planning to use any different value for fielddatacache use case?
There was a problem hiding this comment.
Seems strange to add an arbitrary 500 mb floor for a convenience method, right? Shouldn't we just make it as general as possible and users can choose what floor they need? Either that or only having maximum with a minimum of 0 makes sense to me.
|
❌ Gradle check result for 9dc7c6b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Peter Alfonsi <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19230 +/- ##
============================================
+ Coverage 72.80% 72.84% +0.04%
- Complexity 69599 69678 +79
============================================
Files 5656 5656
Lines 320002 320027 +25
Branches 46338 46343 +5
============================================
+ Hits 232968 233132 +164
+ Misses 68140 68042 -98
+ Partials 18894 18853 -41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Adds convenience methods allowing min/max values for MemorySizeSetting (settings which are parsed from either byte value strings like "500kb" or percentages of the total heap size like "1%"). The min/max arguments are strings which are parsed by the same parser as the default value, so they can also be either byte value strings or percentage strings.
Related Issues
Didn't raise an issue for this as it's pretty minor, but came up in review for #19152. Separated it out as it was not really that related to the original PR and required its own tests, etc.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.