Skip to content

doc: document that addMembership must be called once in a cluster - #23746

Closed
jasnell wants to merge 5 commits into
nodejs:masterfrom
jasnell:udp-add-membership-cluster
Closed

doc: document that addMembership must be called once in a cluster#23746
jasnell wants to merge 5 commits into
nodejs:masterfrom
jasnell:udp-add-membership-cluster

Conversation

@jasnell

@jasnelljasnell commented Oct 19, 2018

Copy link
Copy Markdown
Member

Fixes: #12572
Refs: #16240

Checklist

@nodejs-github-bot

nodejs-github-bot commented Oct 19, 2018

Copy link
Copy Markdown
Collaborator

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1291/pipeline

@nodejs-github-botnodejs-github-bot added dgram Issues and PRs related to the dgram subsystem / UDP. doc Issues and PRs related to the documentations. labels Oct 19, 2018
Trott
Trott reviewed Oct 19, 2018
View reviewed changes
Comment threaddoc/api/dgram.md Outdated
available interface, call `addMembership` multiple times, once per interface.

When sharing a UDP socket across multiple `cluster` workers, the
`socket.addMembership()` function must only be called *only once* or an

@TrottTrottOct 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is one too many occurrences of the word only.

FWIW, I'd avoid the italics here too.

@jasnelljasnellOct 19, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Feel free to make any edits you'd like

@BridgeARBridgeAROct 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is the new github feature to make suggestions that add a commit by accepting the suggestion :-)

@vsemozhetbytvsemozhetbytOct 19, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I am not mistaken, any Collaborator can assign themselves to a PR and accept an own proposed change. This can alleviate nit addressing for the PR author, but also can be felt like a bit arbitrarily thing. But if a PR author declares concent, this seems OK.

Trott
Trott reviewed Oct 19, 2018
View reviewed changes
Comment threaddoc/api/dgram.md Outdated
```js
const cluster = require('cluster');
const dgram = require('dgram');
if (cluster.isMaster) {

@TrottTrottOct 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Linter is failing because of indentation issues on this line and the next two lines.

lpinca
lpinca approved these changes Oct 19, 2018
View reviewed changes

@lpincalpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with nits addressed.

@jasnelljasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 19, 2018
@vsemozhetbyt

vsemozhetbyt commented Oct 19, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Experiment: I am trying to assign myself to the PR and accept my own nit change proposal (which === pushing to the PR branch?).

@vsemozhetbytvsemozhetbyt self-assigned this Oct 19, 2018
vsemozhetbyt
vsemozhetbyt reviewed Oct 19, 2018
View reviewed changes
Comment threaddoc/api/dgram.md Outdated
Comment threaddoc/api/dgram.md Outdated
vsemozhetbytand others added 2 commits October 19, 2018 07:58
@vsemozhetbyt@jasnell
[Squash] nit
771a5e0
Co-Authored-By: jasnell <jasnell@gmail.com>
@vsemozhetbyt@jasnell
[Squash] nit
9e8e181
Co-Authored-By: jasnell <jasnell@gmail.com>
@jasnell

jasnell commented Oct 19, 2018

Copy link
Copy Markdown
MemberAuthor

Oh! Sorry @vsemozhetbyt ... I accepted the changes then saw you note about trying an experiment... hopefully I didn't mess it up

@vsemozhetbyt

vsemozhetbyt commented Oct 19, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Fails: I can see "Apply suggestion" button, But I cannot apply the change:

Screenshots:

fx1
fx2


@vsemozhetbyt

vsemozhetbyt commented Oct 19, 2018

Copy link
Copy Markdown
Contributor

Not sure if this was a race condition, we can try it again later in some other PR)

@vsemozhetbytvsemozhetbyt removed their assignment Oct 19, 2018
@jasnelljasnell added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 19, 2018
@jasnell

jasnell commented Oct 19, 2018

Copy link
Copy Markdown
MemberAuthor

Please 👍 to fast track

@vsemozhetbytvsemozhetbyt mentioned this pull request Oct 19, 2018
Closed
jasnell added a commit that referenced this pull request Oct 23, 2018
@jasnell
doc: document that addMembership must be called once in a cluster
30d42f6
Fixes: #12572
Refs: #16240
PR-URL: #23746
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@jasnell

jasnell commented Oct 23, 2018

Copy link
Copy Markdown
MemberAuthor

Landed in 30d42f6

@jasnelljasnell closed this Oct 23, 2018
targos pushed a commit that referenced this pull request Oct 24, 2018
@jasnell@targos
doc: document that addMembership must be called once in a cluster
83b776c
Fixes: #12572
Refs: #16240
PR-URL: #23746
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targostargos mentioned this pull request Oct 27, 2018
Merged
@beevelopbeevelop mentioned this pull request Nov 2, 2018
Closed
This was referenced Nov 2, 2018
Closed
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
@jasnell@MylesBorins
doc: document that addMembership must be called once in a cluster
67fa459
Fixes: #12572
Refs: #16240
PR-URL: #23746
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
@jasnell@MylesBorins
doc: document that addMembership must be called once in a cluster
64f9a3c
Fixes: #12572
Refs: #16240
PR-URL: #23746
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
@jasnell@MylesBorins
doc: document that addMembership must be called once in a cluster
56ba6ee
Fixes: #12572
Refs: #16240
PR-URL: #23746
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@codebyterecodebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@jasnell@rvagg
doc: document that addMembership must be called once in a cluster
96a4417
Fixes: #12572
Refs: #16240
PR-URL: #23746
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@jasnell@rvagg
doc: document that addMembership must be called once in a cluster
32ae851
Fixes: #12572
Refs: #16240
PR-URL: #23746
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@jasnell@rvagg
doc: document that addMembership must be called once in a cluster
a3362a7
Fixes: #12572
Refs: #16240
PR-URL: #23746
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@jasnell@MylesBorins
doc: document that addMembership must be called once in a cluster
32df77c
Fixes: #12572
Refs: #16240
PR-URL: #23746
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@codebyterecodebytere mentioned this pull request Nov 29, 2018
Merged
@BethGriggsBethGriggs mentioned this pull request Dec 4, 2018
Merged
This was referenced Dec 18, 2018
Closed
Closed
Closed
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
@jasnell@MylesBorins
doc: document that addMembership must be called once in a cluster
3c4949a
Fixes: #12572
Refs: #16240
PR-URL: #23746
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@jasnell@rvagg
doc: document that addMembership must be called once in a cluster
5f5ab97
Fixes: #12572
Refs: #16240
PR-URL: #23746
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Mar 7, 2019
@jasnell@BethGriggs
doc: document that addMembership must be called once in a cluster
ad62971
Fixes: #12572
Refs: #16240
PR-URL: #23746
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Mar 15, 2019
Merged
@beevelopbeevelop mentioned this pull request Apr 3, 2019
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott left review comments
@BridgeARBridgeARBridgeAR left review comments
@lpincalpincalpinca approved these changes
@trivikrtrivikrtrivikr approved these changes
+2 more reviewers
@vsemozhetbytvsemozhetbytvsemozhetbyt left review comments
@refackrefackrefack approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.dgramIssues and PRs related to the dgram subsystem / UDP.docIssues and PRs related to the documentations.fast-trackPRs that do not need to wait for 48 hours to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jasnell@nodejs-github-bot@vsemozhetbyt@refack@Trott@lpinca@BridgeAR@trivikr@MylesBorins