-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[service] Remove getBallastSize from service #10696
[service] Remove getBallastSize from service #10696
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10696 +/- ##
==========================================
- Coverage 92.39% 92.24% -0.15%
==========================================
Files 403 403
Lines 18744 18718 -26
==========================================
- Hits 17318 17267 -51
- Misses 1066 1096 +30
+ Partials 360 355 -5 ☔ View full report in Codecov by Sentry. |
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.
On #8342 (comment) we discussed adding a new metric with the ballast size to help users migrate. I am fine not doing this given the ballast extension has been deprecated for a long time, but just bringing it up again in case we want to do that
I'd say since the helm chart has not been using the extension for six months, the value of |
@@ -168,16 +159,6 @@ func getMemUsageChecker(cfg *Config, logger *zap.Logger) (*memUsageChecker, erro | |||
func (ml *MemoryLimiter) readMemStats() *runtime.MemStats { | |||
ms := &runtime.MemStats{} | |||
ml.readMemStatsFn(ms) | |||
// If proper configured ms.Alloc should be at least ml.ballastSize but since | |||
// a misconfiguration is possible check for that here. | |||
if ms.Alloc >= ml.ballastSize { |
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.
Can we add a separate changelog entry for this change?
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.
added one for each memory limiter
Description
This PR removes all ballast logic from service. This effectively deprecates the ballastextension as including the extension with this service would do nothing.
Related to #10671
Link to tracking issue
Closes #8342
Testing
Unit tests.