Move gce reserved to interface #4511
Conversation
k8s-ci-robot
added
cncf-cla: yes
yaroslava-serdiuk
force-pushed
the
templates
branch
3 times, most recently
from
cb1d88d to
3195d87
Compare
| "strconv" | ||
| "strings" | ||
| // Reserved calculates the OS reserved values. | ||
| type Reserved interface { |
There was a problem hiding this comment.
I'd prefer a more descriptive name. Maybe 'NodeReservedCalculator'? Or 'ReservedResoucesCalculator'?
There was a problem hiding this comment.
Renamed to OsReservedCalculator
| // Reserved calculates the OS reserved values. | ||
| type Reserved interface { | ||
| // CalculateKernelReserved computes how much memory Linux kernel will reserve. | ||
| CalculateKernelReserved(physicalMemory int64, os OperatingSystem, osDistribution OperatingSystemDistribution, nodeVersion string) int64 |
There was a problem hiding this comment.
Current implementation doesn't use the nodeVersion parameter and the code calling it passes empty string. Should the interface (via docstring) specify some expectations here? Ex. by explicitly saying nodeVersion is optional and clarifying the expected behavior if not specified?
There was a problem hiding this comment.
Added a comment that it is optional.
Ex. by explicitly saying nodeVersion is optional and clarifying the expected behavior if not specified?
behavior depends on implementation, so not sure what to say.
yaroslava-serdiuk
force-pushed
the
templates
branch
from
3195d87 to
fba9dab
Compare
k8s-ci-robot
added
size/M
| // OsReservedCalculator calculates the OS reserved values. | ||
| type OsReservedCalculator interface { | ||
| // CalculateKernelReserved computes how much memory OS kernel will reserve. | ||
| // NodeVersion parameter may be optional if it is not used in calculation. |
There was a problem hiding this comment.
As a user of the interface I don't know if the field is used in calculation or not. What I had in mind was along the lines of 'NodeVersion parameter is optional. If left empty a result calculated using default node version will be returned', ie. setting requirements for how any implementation is expected to handle empty value.
yaroslava-serdiuk
force-pushed
the
templates
branch
from
fba9dab to
fb243c4
Compare
yaroslava-serdiuk
force-pushed
the
templates
branch
from
fb243c4 to
237b152
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MaciekPytel, yaroslava-serdiuk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
No description provided.