cluster: remove deprecated API #13702

jasnell · 2017-06-15T15:47:49Z

This is an alternative to #13684

The deprecation code is preserved and the messaging around the reasoning for the deprecation updated.
The change is split into multiple commits to allow easier backporting of selected changes (see cluster: update deprecated API #13704)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

cluster

isaacs

(I don't think I actually have blocking rights here, but I do have a change to request.)

isaacs · 2017-06-15T16:44:51Z

doc/api/deprecations.md

@@ -95,12 +95,18 @@ methods, the `options.customFds` option is deprecated. The `options.stdio`
 option should be used instead.

 <a id="DEP0007"></a>
-### DEP0007: cluster worker.suicide


If this word continues to appear in this file, it should have a content warning at the top, and/or a content warning and a click-through to view this section.

I'm fine with a content warning within this line item (where it's more likely to be seen when someone clicks through to find the information for the specific deprecation code. However, it would be helpful if you had a specific suggestion for the wording of such a warning.

Would it be possible to not include worker.suicide inside the error message but rather a reference to the documentation?

### DEP0007: The api used is deprecated, please replace with worker.exitedAfterDisconnect. Please refer to documentation for more information.

In the extended documentation we could start with

Content warning: Self Harm. Please click here to skip to the next entry.

We can then make "click here" an href to the next header

the name of the property is not included in the warning message that is printed to the console., it is included only in this one deprecations.md document. I'd rather not make it any more complicated than that.

This would be an improvement:

<a id="DEP0007"></a> **Content Warning**: self harm. [Click here to skip to the next entry.](#DEP00008) ----- ### DEP0007: Replace cluster worker.suicide with worker.exitedAfterDisconnect

A much better approach would add a CSS rule like this:

#DEP0007Content { visibility: hidden } #DEP0007Content:target { visibility: visible }

and then

<a id="DEP0007"></a> **Content Warning**: self harm. [Click to show](#DEP0007Content) <div id="DEP0007Content"> ...

I realize this adds some complexity, but it's not like it's something we'll have to do very often, and it seems like a very reasonable cost for the benefit it provides.

Shoot. I just now saw this comment come through @isaacs .... of course it was immediately after landing. This specific additional change can be handled and discussed via a separate PR. Sorry about missing it.

@jasnell No problem. Improvements can come iteratively :) Do you want me to take a crack at writing that update, or do you want to tackle it?

mcollina

LGTM

seishun · 2017-06-15T20:27:47Z

doc/api/deprecations.md

+In an earlier version of the Node.js `cluster`, an unfortunate decision was
+made to add a boolean property with the name `suicide` to the `Worker` object.
+The intent of this property was to provide an indication of how and why the
+`Worker` instance exited. The naming of the property is unfortunate not only


I disagree with giving sentimental assessment ("unfortunate") to the naming of the property here. It should be a purely technical explanation of the change.

For that, I disagree. One thing we've have consistently needed to get better at is explaining why things are deprecated and removed.

I think you misunderstood. I'm not arguing against explaining why. My gripe is with the word "unfortunate" and the apologetic tone.

Regardless of how one feels about "unfortunate", I do think this text:

The naming of the property is unfortunate not only because it is not entirely accurate of the exit semantics, but also because of the potential emotional impact the word carries. In Node.js 6.0.0, the old property was deprecated and replaced with a new [worker.exitedAfterDisconnect][] property.

...would be better as:

In Node.js 6.0.0, the old property was deprecated and replaced with a new [worker.exitedAfterDisconnect][] property. The old property name was inaccurate and unnecessarily emotion-laden.

(If nothing else, please clean up the awkward "accurate of the exit semantics" phrase.)

I just updated the text but I like your suggestion here better @Trott

Updated to use the suggested wording

seishun · 2017-06-15T21:30:13Z

doc/api/deprecations.md

+`suicide` was added to the `Worker` object. The intent of this property was to
+provide an indication of how and why the `Worker` instance exited. In Node.js
+6.0.0, the old property was deprecated and replaced with a new
+[worker.exitedAfterDisconnect][] property. The old property name was inaccurate


Minor nit: I don't think a property name can be "inaccurate", how about "misleading"?

I think inaccurate is preferable to misleading here.

"The name is inaccurate" doesn't seem to be...uh...inaccurate. :-D

"The name is misleading" seems a bit loaded and I think less accurate.

Other options, although I'm fine with inaccurate: imprecise, confusing, difficult to construe, easy to misunderstand

Yeah, I'm not sure that misleading is any better. imprecise could work.

My point is that the name of a property doesn't need to be 100% descriptive, it's usually kind of a mnemonic. In a sense, all property names are "imprecise" to some extent. In case of the property in question, the meaning of the word is quite different from the purpose of the property, which makes it easy to misinterpret/misremember.

inappropriate? Not appropriate for node in general & not an appropriate description of the status.
Or simply not good

incomplete? inadequate?

We could also just remove the inaccurate part entirely as it's not the primary reason for removal AFAICT.

If we want to get better at explaining why we removed something, as @jasnell implores us to do, being direct is probably best anyway:

The old property name was unnecessarily emotion-laden.

jasnell · 2017-06-19T22:24:19Z

ping @nodejs/ctc

jasnell · 2017-06-19T22:25:24Z

Given the sign off, I'd like to get this landed by tomorrow, but want to make sure there are no objections
@isaacs ... please see my response to your review here: #13702 (comment)

seishun · 2017-06-20T06:32:54Z

@jasnell Could you give your opinion on my comment here?

jasnell · 2017-06-20T16:12:10Z

@seishun @Trott ... I've updated the language in the docs a bit.

jasnell · 2017-06-20T16:13:10Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/8761/

seishun · 2017-06-20T16:38:59Z

@jasnell Much better now, but I think "did not precisely describe the actual semantics" is an understatement: the actual semantics are pretty much the opposite of what people would expect. (see https://youtu.be/jJaIwea8r2A?t=422) Maybe replace "precisely" with "adequately", or drop it altogether?

Also, I don't think the word itself is emotion-laden. While it can affect people emotionally, it doesn't carry emotion on its own (unlike e.g. "tragedy"), it's just a plain term. Maybe something like "has unnecessary negative connotations" would be better?

isaacs · 2017-06-20T17:16:52Z

doc/api/deprecations.md


-Type: Runtime
+Type: End-of-Life


Rather unfortunate terminology here, in context. I don't have a better suggestion though :\

We could replace all instances of End-of-Life with Broken and/or Removed, that might be more accurate terminology anyway (but we can also keep that for a follow-up, that might be easier).

This is a fixed term as defined in deprecations.md, so I don't think we want to change this.

I noted the irony also. To change this we would need a change to the deprecation policy as a whole, and the usage here is consistent with general usage in the industry. That's not to say it's particularly good, just that for now there isn't a better option.

@tniessen I know, as I said I’m suggesting that we change the terminology, not this single instance.

@addaleax We both wrote our comments within a minute, I did not see your comment until I had written mine, I was not responding to you ;)

jasnell · 2017-06-20T18:19:31Z

CI is good. Only failure is unrelated.

jasnell · 2017-06-20T18:25:25Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/882/

@nodejs/ctc ... I will be landing this shortly.

Can address feedback separately once specific additional edits are suggested

PR-URL: #13702 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell · 2017-06-20T18:52:04Z

Landed in 3fab9f2, eaaec57, and 1fcb76e

refack · 2017-06-20T18:58:18Z

Landed in 3fab9f2, eaaec57, and 1fcb76e

Not a :tada moment so 💂‍♂️💂‍♂️💂‍♂️

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 15, 2017

jasnell added this to the 9.0.0 milestone Jun 15, 2017

refack approved these changes Jun 15, 2017

View reviewed changes

sam-github approved these changes Jun 15, 2017

View reviewed changes

vsemozhetbyt added the cluster Issues and PRs related to the cluster subsystem. label Jun 15, 2017

This was referenced Jun 15, 2017

cluster: tweak deprecation #13703

Closed

cluster: remove deprecated problematic API #13684

Closed

jasnell mentioned this pull request Jun 15, 2017

cluster: update deprecated API #13704

Closed

4 tasks

isaacs previously requested changes Jun 15, 2017

View reviewed changes

tniessen approved these changes Jun 15, 2017

View reviewed changes

mcollina approved these changes Jun 15, 2017

View reviewed changes

cjihrig approved these changes Jun 15, 2017

View reviewed changes

addaleax approved these changes Jun 15, 2017

View reviewed changes

seishun reviewed Jun 15, 2017

View reviewed changes

jasnell force-pushed the remove-deprecated-api branch 2 times, most recently from 375600b to 9d10355 Compare June 15, 2017 21:28

seishun reviewed Jun 15, 2017

View reviewed changes

jasnell added the ctc-review label Jun 19, 2017

jasnell added 3 commits June 20, 2017 09:10

doc: EOL deprecated API and update notes

14930b5

test: use worker.exitedAfterDisconnect consistently

f1888bc

cluster: remove deprecated property

6400a52

jasnell force-pushed the remove-deprecated-api branch from 9d10355 to 6400a52 Compare June 20, 2017 16:11

isaacs reviewed Jun 20, 2017

View reviewed changes

jasnell closed this Jun 20, 2017

Trott removed the ctc-review label Jun 25, 2017

refack mentioned this pull request Aug 7, 2017

meta: handling of undocumented endpoint #14679

Closed

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018


		Type: Runtime
		Type: End-of-Life

cluster: remove deprecated API #13702

cluster: remove deprecated API #13702

Conversation

jasnell commented Jun 15, 2017 • edited Loading

Checklist

Affected core subsystem(s)

isaacs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacs Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

jasnell commented Jun 19, 2017

jasnell commented Jun 19, 2017

seishun commented Jun 20, 2017

jasnell commented Jun 20, 2017

jasnell commented Jun 20, 2017

seishun commented Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jun 20, 2017

jasnell commented Jun 20, 2017

jasnell commented Jun 20, 2017

refack commented Jun 20, 2017

jasnell commented Jun 15, 2017 •

edited

Loading

isaacs Jun 20, 2017 •

edited

Loading

refack Jun 20, 2017 •

edited

Loading

Trott Jun 20, 2017 •

edited

Loading

seishun commented Jun 20, 2017 •

edited

Loading