-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Only resolve PodSpec if needed (lazy resolution) #7259
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for your contribution! 🙏 Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected. While you are waiting, make sure to:
Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient. Learn more about our contribution guide. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Signed-off-by: Dima Altukhov <[email protected]>
ff658a7 to
66ffe65
Compare
|
/run-e2e |
|
I think that avoid checking not needed fields is always nice, but why would you like to avoid podSpec? Is there any case you're dealing with? |
Yes! After update from version 2.16.1 to 2.18.1 i noticed a spike in memory usage from 256Mb to 1Gb! In our clusters we do not use any I would like to understand why/when behaviour changed? Maybe i missed changelog
|
|
Gotcha! now makes sense :) |
JorTurFer
left a comment
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.
nice improvement!
|
Could you include a record in changelog? I think that under improvements it's nice |
|
/run-e2e |
|
It looks that unit tests are failing, could you take a look? |
zroubalik
left a comment
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.
Nice improvement!
Could you please check failures in unit tests and add Changelog entry?
Signed-off-by: Dmitriy Altuhov <[email protected]>
Signed-off-by: Dmitriy Altuhov <[email protected]>
Signed-off-by: Dmitriy Altuhov <[email protected]>
|
Given that this PR apparently no longer fixes the memory usage issue, and given @JorTurFer 's comment about using a cached client, is this PR still necessary as an improvement? Is there any benefit to this PR, does it still solve something? |
|
Although the client is cached, requesting items which are not going to be used, increases the memory usage (IIRC, runtime k8s client uses DeepCopy from cached items to ensure consistency), I'm not sure if this makes sense or it's a micro optimization which makes the code harder to understand or really makes sense. @alt-dima , could you test rebasing your branch with latest dep versions and check if there is any memory reduction from v2.18.2? |




Provide a description of what has been changed
Checklist
Fixes #
Relates to #