Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
jasnell wants to merge 5 commits into from
Closed

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

jasnell wants to merge 5 commits into from

Conversation

@jasnell
Copy link
Member

@jasnelljasnell commented Oct 19, 2018

Fixes: #12572
Refs: #16240

Checklist

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 19, 2018

@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
doc/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
Copy link
Member

@TrottTrottOct 19, 2018

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.

Copy link
MemberAuthor

@jasnelljasnellOct 19, 2018

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

Copy link
Member

@BridgeARBridgeAROct 19, 2018

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 :-)

Copy link
Contributor

@vsemozhetbytvsemozhetbytOct 19, 2018•
edited
Loading

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
doc/api/dgram.md Outdated
```js
constcluster=require('cluster');
constdgram=require('dgram');
if (cluster.isMaster){
Copy link
Member

@TrottTrottOct 19, 2018

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
Copy link
Member

@lpincalpinca left a comment

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
Copy link
Contributor

vsemozhetbyt commented Oct 19, 2018•
edited
Loading

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
vsemozhetbytand others added 2 commits October 19, 2018 07:58
@vsemozhetbyt@jasnell
[Squash] nit
771a5e0
Co-Authored-By: jasnell <[email protected]>
@vsemozhetbyt@jasnell
[Squash] nit
9e8e181
Co-Authored-By: jasnell <[email protected]>
@jasnell
Copy link
MemberAuthor

jasnell commented Oct 19, 2018

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

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Oct 19, 2018•
edited
Loading

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

Screenshots:

fx1
fx2


@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Oct 19, 2018

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
Copy link
MemberAuthor

jasnell commented Oct 19, 2018

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
@jasnell
Copy link
MemberAuthor

jasnell commented Oct 23, 2018

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
@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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
@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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
@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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
@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