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

Configurable CONCURRENT_ACTIONS #850

Merged
merged 1 commit into from
Mar 23, 2015
Merged

Conversation

kaaelhaa
Copy link

In our environment we have made the constant CONCURRENT_ACTIONS a configuration variable as this can significantly improve reload times when you have 8 processes of the same app.

The default behaviour is, and should be, to only do one concurrent action. But for power users and environments where it make sense (e.g. can tolerate the overhead of extra processes running while old workers are finishing their jobs in gracefulReload-scenarios) this should be configurable.

@hypesystem
Copy link

I approve of this idea!

@kaaelhaa
Copy link
Author

So, @Unitech is this PR something you will consider merging (after rebase) or should I close this and continue using our custom fork of PM2?

@jorge-d
Copy link
Contributor

jorge-d commented Mar 16, 2015

Hi @kaaelhaa, this looks OK to me, could you handle the rebase ?

Thanks,

@Unitech
Copy link
Owner

Unitech commented Mar 17, 2015

This could hurt some things of the PM2 core and remove rolling restarts, I don't recommend overriding this value

@hypesystem
Copy link

"could"?

@kaaelhaa
Copy link
Author

Rebased.

@Unitech From our usage and inspection of the code base I cannot see why this should hurt anything.

It is a power user feature and should be treated as such by having a decent default value. If a user has set -i max when starting a process, the underlying hardware has to be capable of handling the extra context-switching and workload for a short period of time (until old processes has finished).

If you are uncertain about some use cases, please let me know so I can have them tested.

@jorge-d
Copy link
Contributor

jorge-d commented Mar 18, 2015

@jshkurti Any thoughts on this?

@jshkurti
Copy link
Contributor

I'm going to refactor a bit reload/gracefulReload soon.
I will keep this PR in mind while doing that and see what is the best solution.
(Chances are high this will be merged unless heavy stability issues, but then, the user still has the choice to leave the variable at its default value of 1)
I'll keep you informed ;)

@jshkurti jshkurti self-assigned this Mar 19, 2015
@jshkurti
Copy link
Contributor

I'm merging this because hey, why the heck not, if you're willing to take the risk, you should be able to do so and keep using the latests versions of PM2 :)
But I won't talk about it in the README because I tried with a value of 4 and some tests didn't pass.

jshkurti added a commit that referenced this pull request Mar 23, 2015
@jshkurti jshkurti merged commit 80ad9d5 into Unitech:master Mar 23, 2015
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.

5 participants