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

cluster: fix inspector port assignment by santigimeno · Pull Request #18696 · nodejs/node · GitHub

/ node Public

cluster: fix inspector port assignment - #18696

Closed
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:fix_inspector_port_cluster
ConversationCommits (1)ChecksFiles changed
Closed

cluster: fix inspector port assignment#18696
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:fix_inspector_port_cluster

Conversation

santigimeno commented Feb 10, 2018

Copy link
Copy Markdown
Member

Make sure that inspector ports in cluster are inside the valid range:
[1024, 65535].
Fix flaky test-inspector-port-zero-cluster.

Fixes: #18303

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)

nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Feb 10, 2018

santigimeno commented Feb 10, 2018

Copy link
Copy Markdown
Member Author

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

ChALkeR reviewed Feb 10, 2018
View reviewed changes
Comment thread lib/internal/cluster/master.js Outdated

ChALkeR Feb 10, 2018 •
edited
Loading

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

I would have written this as:

const [minPort, maxPort] = [1024, 65535];
…
    if (inspectPort > maxPort)
      inspectPort = inpectPort - maxPort + minPort - 1;

Not sure if others agree with me, though, so lets wait for further comments ;-)

daynin Feb 10, 2018

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

@ChALkeR looks a little bit more readable :)

daynin commented Feb 10, 2018

Copy link
Copy Markdown
Contributor

@santigimeno hi! You have a little typo in the title "inpector" -> "inspector"

tniessen changed the title cluster: fix inpector port assignment cluster: fix inspector port assignment Feb 11, 2018
cjihrig approved these changes Feb 13, 2018
View reviewed changes

cjihrig left a comment

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

LGTM with nits addressed.

Comment thread test/sequential/test-inspector-port-zero-cluster.js Outdated

cjihrig Feb 13, 2018

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

I think I'd prefer to keep the existing assertion style instead of using a forEach(). It's much more readable IMO.

santigimeno force-pushed the fix_inspector_port_cluster branch from 6ce83ac to 1af082e Compare February 18, 2018 10:17
cluster: fix inspector port assignment
efdea32 
Make sure that inspector ports in cluster are inside the valid range:
`[1024, 65535]`.
Fix flaky `test-inspector-port-zero-cluster`.

Fixes: nodejs#18303
santigimeno force-pushed the fix_inspector_port_cluster branch from 1af082e to efdea32 Compare February 18, 2018 10:17

santigimeno commented Feb 18, 2018

Copy link
Copy Markdown
Member Author

Comments addressed. CI: https://ci.nodejs.org/job/node-test-pull-request/13249/

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 18, 2018

BridgeAR commented Feb 22, 2018

Copy link
Copy Markdown
Member

Landed in 45982de 🎉

BridgeAR closed this Feb 22, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 22, 2018
cluster: fix inspector port assignment
45982de 
Make sure that inspector ports in cluster are inside the valid range:
`[1024, 65535]`.
Fixes flaky `test-inspector-port-zero-cluster`.

PR-URL: nodejs#18696
Fixes: nodejs#18303
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to addaleax/node that referenced this pull request Feb 26, 2018
cluster: fix inspector port assignment
dfe0bc1 
Make sure that inspector ports in cluster are inside the valid range:
`[1024, 65535]`.
Fixes flaky `test-inspector-port-zero-cluster`.

PR-URL: nodejs#18696
Fixes: nodejs#18303
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
cluster: fix inspector port assignment
4fa1f31 
Make sure that inspector ports in cluster are inside the valid range:
`[1024, 65535]`.
Fixes flaky `test-inspector-port-zero-cluster`.

PR-URL: #18696
Fixes: #18303
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax mentioned this pull request Feb 27, 2018
Closed
MylesBorins pushed a commit that referenced this pull request Apr 13, 2018
cluster: fix inspector port assignment
bf4d074 
Make sure that inspector ports in cluster are inside the valid range:
`[1024, 65535]`.
Fixes flaky `test-inspector-port-zero-cluster`.

PR-URL: #18696
Fixes: #18303
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
cluster: fix inspector port assignment
4ccbab0 
Make sure that inspector ports in cluster are inside the valid range:
`[1024, 65535]`.
Fixes flaky `test-inspector-port-zero-cluster`.

PR-URL: nodejs#18696
Fixes: nodejs#18303
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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

ChALkeR ChALkeR left review comments
cjihrig cjihrig approved these changes
BridgeAR BridgeAR approved these changes
+1 more reviewer
daynin daynin left review comments
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. cluster Issues and PRs related to the cluster subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cluster,test: failing cluster+inspector test when using port 65535

7 participants

Back | FazBrowse Home | New Git URL