-
Notifications
You must be signed in to change notification settings - Fork 443
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 system loader setting for file descriptor limit #954
Conversation
…t (default 1024).
… Default for all loaders is 1024.
Hi @levinson, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
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.
Thanks a lot. Looks like a solid pull request. Some small comments that can be easily fixed and then this is good to merge 👍
@@ -158,6 +162,7 @@ object SystemloaderPlugin extends AutoPlugin { | |||
"stop_runlevels" -> stopRunlevels.getOrElse(""), | |||
"start_facilities" -> startOn.getOrElse(""), | |||
"stop_facilities" -> stopOn.getOrElse(""), | |||
"file_descriptor_limit" -> fileDescriptorLimit.getOrElse(""), |
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.
What happens if ulimit
is called with no arguments? Will this result in unlimited
?
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.
If ulimit is called with no arguments it assumes -f
and will not set limits, instead it will return the value of The maximum size of files created by the shell
.
From the man pages: http://linuxcommand.org/man_pages/ulimit1.html
If limit is given, it is the new value of the specified resource
(the -a option is display only). If no option is given, then -f
is assumed. Values are in 1024-byte increments, except for -t,
which is in seconds, -p, which is in units of 512-byte blocks,
and -n and -u, which are unscaled values. The return status is
0 unless an invalid option or argument is supplied, or an error
occurs while setting a new limit.
@@ -50,6 +51,7 @@ object SystemloaderPlugin extends AutoPlugin { | |||
def systemloaderSettings: Seq[Setting[_]] = Seq( | |||
serverLoading := None, | |||
serviceAutostart := true, | |||
fileDescriptorLimit := Some("1024"), |
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 this should go into the LinuxPlugin
. At somepoint I will refactor this into the SystemloaderPlugin
where it belongs, but for this PR it's fine to put it into the LinuxPlugin
@@ -49,6 +49,9 @@ General Settings | |||
``retryTimeout`` | |||
Timeout between retries in seconds | |||
|
|||
``fileDescriptorLimit`` | |||
Maximum number of open file descriptors for the spawned application. The default value is 1024. |
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.
❤️
Awesome work. Thanks |
Thanks for the quick review and merge! |
There currently is not a good way to increase the file descriptor limit for system loaders. I have added a new system loader key
fileDescriptorLimit
with support for upstart, systemd and systemv loaders, which can be overridden to change the default limit of 1024.