-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Code refactor: remove a function scope variable err for psi stats #3938
Conversation
Signed-off-by: lifubang <lifubang@acmcoder.com>
ci lint error:
The |
@@ -91,3 +91,11 @@ func statCpu(dirPath string, stats *cgroups.Stats) error { | |||
} | |||
return nil | |||
} | |||
|
|||
func statCpuPressure(dirPath string, stats *cgroups.Stats) error { | |||
const file = "cpu.pressure" |
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.
I would pass "cpu.pressure" directly, without adding an intermediary const
. The const
would make sense if we use it more than once, or if the value is complicated/long -- neither is the case here.
I'm neutral on this.
Left a nit about using consts for file names, but overall LGTM |
Re: linter, I think we can start naming things better ( @thaJeztah WDYT? |
func statIoPressure(dirPath string, stats *cgroups.Stats) error { | ||
const file = "io.pressure" | ||
var err error | ||
|
||
stats.BlkioStats.PSI, err = statPSI(dirPath, file) | ||
return err | ||
} |
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.
I'd rewrite this one to
func statIoPressure(dirPath string, stats *cgroups.Stats) (Err error) {
stats.BlkioStats.PSI, Err = statPSI(dirPath, "io.pressure")
return
}
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.
but with this rewrite, it doesn't make much sense to have it as a separate function, right?
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.
I don't think this makes the code much better than it is.
Address #3900 (comment)
I think it's unnecessary to add a function scope variable suddenly. We should keep the code style like other cgroup stats.