-
Notifications
You must be signed in to change notification settings - Fork 56
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
Make CPU Limits to point correctly, earlier it's pointing to cpu request #1023
Conversation
Signed-off-by: bharathappali <[email protected]>
Thanks @khansaad for pointing the issue |
Good catch indeed @khansaad!! |
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.
Do we need to do this calculation in both the engines? Can this be done outside in common code
@dinogun It is jus calculating a difference so I have added it in the engines, Can be extracted and common out. |
Yes, calculations are done in both the engines.
Yes can be done, but after a detailed inspection I felt it would be better to have these calculations done in the respective engines as these calculations are tightly coupled to populating the notifications and also lot many objects need to be sent to the common code to take decisions and calculate the differences, Moreover these engines using the data for representational purpose as of now, but in future if engine needs to take some actions or need to tweak certain things in the data it would be hard to do it in common code as we need to pass the engine name and implement engine specific behaviour over there which might be not correct considering the abstractions built around engines to do the engine specific work in their scope. Please lemme know your thoughts on this. |
ok, we can schedule the refactoring for a later time then |
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.
LGTM
As mentioned in issue #1022 the variation is not matching the difference between current and recommended value. The issue is due to the incorrect mapping of cpu limits to cpu requests.
@dinogun Can I please have your review?