-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
cluster: migrate from worker.suicide #3743
Conversation
LGTM |
Test suite passes locally fwiw |
if (worker.suicide === true) { | ||
console.log('Oh, it was just suicide\' – no need to worry'). | ||
if (worker.voluntaryExit === true) { | ||
console.log('Oh, it was just voluntary\' – no need to worry'). |
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.
please add exit
, it was just voluntary
-> it was just voluntary exit
LGTM with one comment |
"deprecate" is the wrong term here since you are ensuring exisiting compatibility. maybe "migrating" instead? |
also, maybe not print the deprecation warning until the next major release. |
@@ -427,8 +442,8 @@ function masterInit() { | |||
queryServer(worker, message); | |||
else if (message.act === 'listening') | |||
listening(worker, message); | |||
else if (message.act === 'suicide') |
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.
do you need to watch for the 'suicide' action here to guarantee compatibility?
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.
since the message should only be internal, I don't think so. Unless you or someone else feel differently 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.
makes sense to me 😄
LGTM but we need to be clear about the impact of this, however. It is quite likely a semver-major change (any time we talk about changing event names and deprecating existing terms we likely have to consider it a semver-major). That means getting a deprecation notice into v5 first, then not landing the actual change until v6. |
@jasnell if it's backwards compatible why would it be a semver-major change? |
I said "likely" ;-). The one part of this change that is potentially not backwards compatible is the event that's being triggered (https://github.com/nodejs/node/pull/3743/files#diff-0faa53fc02580d5de2ebb484c41d691cR696). Specifically, are we reasonably certain that this isn't breaking? |
A more boolean terminology like |
@jasnell I believe that those messages are only sent to/from a cluster by the cluster and workers. |
Ok. If we're reasonably sure that it's not breaking, I'm good with semver-minor. |
@Qard I'm fine with either. If you feel strongly about it, I will change it |
return this.voluntaryExit; | ||
}, | ||
set: function(val) { | ||
// TODO: Print deprecation message. |
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.
Should this message include information about when (date or version) to expect deprecation to become removal?
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.
Should be slated for a major version. Honestly, we should just merge this and then open another PR right away with the deprecation warning since master
is v6.
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.
Should I just add a second commit? That way we can cherry-pick the first over to v5 and not the actual deprecation?
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.
yes :)
@ChALkeR I know you didn't feel for it, but could you run some npm analysis on the usage of this API if possible? (Also cc @chrisdickinson) |
As others have said I also think it may be worthwhile to think of a version of |
Although long, if that is the way we want to go, I think |
So long as we can avoid naming like |
Updated to change to |
hrm... I may have misspoke. since we tag the PR to automatically do the version updates we may need two PRs for the two commits since one is semver-minor and one is semver-major. |
Btw, this wasn't backported to 4.x, was it? Perhaps it is too late now as 4.5 already got rcs (cc @thealphanerd), but if there would be 4.6 sometime, it could make sense to backport it the other way around and without the deprecation — i.e. make |
+1 SGTM On Friday, August 5, 2016, Сковорода Никита Андреевич <
|
Because given our LTS timeline, if we don't make 4.x support |
I'll submit a backport PR today |
This is a backport of 4f619bd which migrates from worker.suicide to worker.exitedAfterDisconnect. Related: nodejs#3743
This is a backport of 4f619bd which migrates from worker.suicide to worker.exitedAfterDisconnect. Refs: #3743 PR-URL: #7998 Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Myles Borins <[email protected]>
The defaul value of worker.exitedAfterDisconnect is undefined. If the worker dies on its own the value is changed to false and doesn't remains undefined. While the documentation just mentions 'Set by calling .kill() or .disconnect(). Until then, it is undefined.'. Making the default value as false in worker.js Fixes: nodejs#28837 Refs: nodejs#3743
Fixed the documentaion to reflect the changes in the default value of worker.exitedAfterDisconnect Fixes: nodejs#28837 Refs: nodejs#3743
The test checka for the exit code of the worker. cluster on exit must be called. And the value for exitedAfterDisconnect should be false Fixes: nodejs#28837 Refs: nodejs#3743
fixed the test that checked for default value undefined to false and renamed the test that checks for the value exitedAfterDisconnect if the worker exits on its own. Fixes: nodejs#28837 Refs: nodejs#3743
Fixed the documentation to reflect the changes in the default value of worker.exitedAfterDisconnect Fixes: nodejs#28837 Refs: nodejs#3743
Fixed the documentation to reflect the changes in the default value of worker.exitedAfterDisconnect PR-URL: #29404 Fixes: #28837 Refs: #3743 Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anto Aravinth <[email protected]>
Fixed the documentation to reflect the changes in the default value of worker.exitedAfterDisconnect PR-URL: #29404 Fixes: #28837 Refs: #3743 Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anto Aravinth <[email protected]>
Fixed the documentation to reflect the changes in the default value of worker.exitedAfterDisconnect PR-URL: #29404 Fixes: #28837 Refs: #3743 Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anto Aravinth <[email protected]>
Replace it with worker.exitedAfterDisconnect. Print deprecation message when
getting or setting until it is removed.
Ref: #3721