FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: document that addMembership must be called once in a cluster by jasnell · Pull Request #23746 · nodejs/node · GitHub

/ node Public

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
ConversationCommits (5)ChecksFiles changed
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 commented Oct 19, 2018

Copy link
Copy Markdown
Member

Fixes: #12572
Refs: #16240

Checklist

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-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 reviewed Oct 19, 2018
View reviewed changes
Comment thread 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

Trott Oct 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There is one too many occurrences of the word only.

FWIW, I'd avoid the italics here too.

jasnell Oct 19, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Feel free to make any edits you'd like

BridgeAR Oct 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

vsemozhetbyt Oct 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 reviewed Oct 19, 2018
View reviewed changes
Comment thread doc/api/dgram.md Outdated
```js
const cluster = require('cluster');
const dgram = require('dgram');
if (cluster.isMaster) {

Trott Oct 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lpinca approved these changes Oct 19, 2018
View reviewed changes

lpinca 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with nits addressed.

jasnell added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 19, 2018

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?).

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

jasnell commented Oct 19, 2018

Copy link
Copy Markdown
Member Author

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

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:



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)

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

jasnell commented Oct 19, 2018

Copy link
Copy Markdown
Member Author

Please 👍 to fast track

vsemozhetbyt mentioned this pull request Oct 19, 2018
Closed
jasnell added a commit that referenced this pull request Oct 23, 2018
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 commented Oct 23, 2018

Copy link
Copy Markdown
Member Author

Landed in 30d42f6

jasnell closed this Oct 23, 2018
targos pushed a commit that referenced this pull request Oct 24, 2018
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>
targos mentioned this pull request Oct 27, 2018
Merged
beevelop 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
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
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
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>
codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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
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
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
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>
codebytere mentioned this pull request Nov 29, 2018
Merged
BethGriggs 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
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
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
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>
BethGriggs mentioned this pull request Mar 15, 2019
Merged
beevelop mentioned this pull request Apr 3, 2019
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott left review comments
BridgeAR BridgeAR left review comments
lpinca lpinca approved these changes
trivikr trivikr approved these changes
+2 more reviewers
vsemozhetbyt vsemozhetbyt left review comments
refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. dgram Issues and PRs related to the dgram subsystem / UDP. doc Issues and PRs related to the documentations. fast-track PRs 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

Back | FazBrowse Home | New Git URL