-
Notifications
You must be signed in to change notification settings - Fork 15
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
🫶 Add new SetGoMemLimitWithOptions #9
Conversation
@KimMachineGun, what do you think about it? |
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.
Thank you for your contribution.
The idea seems great!
memlimit/memlimit.go
Outdated
// WithLogger uses the supplied printf implementation for log output. | ||
// By default log.Printf. | ||
func WithLogger(printf func(string, ...interface{})) Option { | ||
return optionFunc(func(cfg *config) { | ||
cfg.printf = printf | ||
}) | ||
} |
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.
The logging patterns will be changed after the Go 1.21 release to use the log/slog
package. (Go 1.21 is expected to be released in August.)
What about separating the logger part to another PR and re-visiting after the Go 1.21 release?
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.
@KimMachineGun, now there is no way to manage logging outside the library and by default the log
package is used. In the new function SetGoMemLimitWithOptions
, I propose to give the user the opportunity to use
your logger (e.g. zap
)
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 think the printf
style logger is not as customizable as slog.Logger. slog.Logger can even handle log level and logging style (in a structured way such as json, key-value style, etc) as well.
So I want to reserve the name WithLogger
for slog.Logger
and avoid adding new API (even in another name) likely to be deprecated soon (after Go 1.21).
Would slog.Logger
not be sufficient to qualify your needs?
@KimMachineGun, ✅ |
@KimMachineGun ,what do you think about it? |
// WithRatio configure memory limit. | ||
// By default `defaultAUTOMEMLIMIT`. | ||
func WithRatio(ratio float64) Option { | ||
return Option(func(cfg *config) { | ||
cfg.ratio = ratio | ||
}) | ||
} | ||
|
||
// WithEnv configure memory limit from environment variable. | ||
// By default `false`. | ||
func WithEnv() Option { | ||
return Option(func(cfg *config) { | ||
cfg.env = true | ||
}) | ||
} |
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.
It seems like WithRatio
and WithEnv
work exclusively for each other.
What about not adding WithEnv
and using the environment variable if WithRatio
is not used?
(i.e. using the environment variable by default, and if the ratio is provided, using it instead.)
@mirecl |
Actually this is my question. Can I use the whole example code as it is? are these options chosen in some preference or do i have to only set one type of variable? |
@t-e-s-tweb |
@mirecl |
quick question, if all the options are imported in go file, then how do they take priority over one another or should only one be used at a time? e.g using env, hardlimit by ram, by ratio or by cgroups? if all are added then which is applied? is there any way to check the currently applied limit? |
@t-e-s-tweb Below is a simplified version of the automemlimit process.
|
I forked it to a new PR for the fast follow-up for Go 1.21. |
while using
i get
|
@KimMachineGun, most likely need a new release (tag). |
@mirecl |
@mirecl |
✅ Add configure options for
SetGoMemLimitWithEnv
:❗For example:
P.S. I am confident that this commit will be beneficial for the community.