Skip to content
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

[3.0] Supervisor nice option #551

Merged
merged 2 commits into from
Mar 20, 2019
Merged

[3.0] Supervisor nice option #551

merged 2 commits into from
Mar 20, 2019

Conversation

halaei
Copy link
Contributor

@halaei halaei commented Mar 19, 2019

Closing #549
This PR increments process niceness on each supervisor via config:

            'supervisor-1' => [
                'connection' => 'redis',
                'queue' => ['default'],
                'balance' => 'simple',
                'processes' => 10,
                'tries' => 3,
                'nice' => 5,
            ],

The supervisor calls proc_nice(5) to set niceness:

I don't know if you can regard it as backward compatible because of the change in supervisor option constructor. Also not sure if it has to be sent to 3.0 or master.

@driesvints
Copy link
Member

It's backwards compatible because adding constructor arguments with defaults won't break anything. This is fine 👍

@driesvints driesvints changed the title supervisor nice option [3.0] Supervisor nice option Mar 19, 2019
@driesvints
Copy link
Member

@halaei Also: always add the full description for this PR to your main comment and not just link to a issue/pull request.

@halaei
Copy link
Contributor Author

halaei commented Mar 19, 2019

@driesvints Updated comment

@halaei
Copy link
Contributor Author

halaei commented Mar 19, 2019

Reading code, I see a --paused option:

{--paused : Start the supervisor in a paused state}

Reading that, I wish there was an option to pause a supervisor "only for a few seconds" before starting to scale for the first time. maybe a --pause=10 option.

@taylorotwell
Copy link
Member

The PHP documentation states that the proc_nice value can also be negative?

@halaei
Copy link
Contributor Author

halaei commented Mar 19, 2019

@taylorotwell It will only work if the process is running as a superuser, otherwise an exception will be raised with this message:

proc_nice(): Only a super user may attempt to increase the priority of a process  

@browner12
Copy link
Contributor

Support for Windows does not exist until PHP 7.2. Do we need to do anything in code to account for that, or will that be a documentation solution?

@halaei
Copy link
Contributor Author

halaei commented Mar 19, 2019

@browner12 This is an optional feature. If it is not supported on an environment you can simply not use it.

@driesvints
Copy link
Member

@browner12 Windows support is lacking in general: #170

Copy link
Contributor

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No happy about the 0 default and not allowing negative values!

*
* @var int
*/
public $nice = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is problematic:

  • a niceness of 0 is a valid value
  • the (linux) default for niceness is usually 10

IMHO the correct way is to use null here.

@@ -23,6 +23,7 @@ class SupervisorCommand extends Command
{--max-processes=1 : The maximum number of total workers to start}
{--min-processes=1 : The minimum number of workers to assign per queue}
{--memory=128 : The memory limit in megabytes}
{--nice=0 : The process niceness}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 is not a good value, see my other comment

@@ -74,6 +75,10 @@ public function handle(SupervisorFactory $factory)
*/
protected function start($supervisor)
{
if ($supervisor->options->nice > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 0 is a valid niceness level
  • so are negative values

This check, see my other comment, should be !== null

It doesn't matter that e.g. negative values are only allowed for root user rights. Don't outsmart the dev who might have run as root for whatever reason, etc.

@halaei
Copy link
Contributor Author

halaei commented Mar 19, 2019

@mfn The argument of proc_nice is the amount of increment to the niceness:

proc_nice ( int $increment ) : bool
proc_nice() changes the priority of the current process by the amount specified in increment.

http://php.net/manual/en/function.proc-nice.php

@halaei
Copy link
Contributor Author

halaei commented Mar 19, 2019

@mfn zero increment means no increment, so no need for null. About letting user to use negative values, that is fine for me, although I don't feel I should use it or run my workers as sudo or use Windows at all ;)

@halaei
Copy link
Contributor Author

halaei commented Mar 19, 2019

Maybe I should change the comments in the code from 'The process niceness' to 'Increment to the process niceness'?

@mfn
Copy link
Contributor

mfn commented Mar 19, 2019

@halaei thanks for correcting me, you're right!

Still, I vote for allowing negative values. It's not your decision how others want to run the code.

@taylorotwell taylorotwell merged commit a5c1b7b into laravel:3.0 Mar 20, 2019
@driesvints
Copy link
Member

This has now been released in v3.1.0

@Einenlum
Copy link

This doesn't seem documented: https://laravel.com/docs/11.x/horizon
Could it be added to the documentation somewhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants