Skip to content
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.

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

cluster: remove the useless parameter for master #29470

Closed
wants to merge 2 commits into from
Closed

cluster: remove the useless parameter for master #29470

wants to merge 2 commits into from

Conversation

SEWeiTung
Copy link

@SEWeiTung SEWeiTung commented Sep 6, 2019

It seems that at 'RoundRobinHandle', the 'addressType' isn't used but just used at 'SharedHandle', so remove this useless parameter and its related files.


  • 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

@nodejs-github-bot nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Sep 6, 2019
@SEWeiTung SEWeiTung changed the title lib: remove the parameter for master cluster: remove the parameter for master Sep 6, 2019
@SEWeiTung SEWeiTung changed the title cluster: remove the parameter for master cluster: remove the useless parameter for master Sep 6, 2019
It seems that at 'RoundRobinHandle', the 'addressType' isn't used but
just used at 'SharedHandle', so remove this useless parameter and its
related files.
@Trott
Copy link
Member

Trott commented Sep 22, 2019

@nodejs/cluster

@jasnell
Copy link
Member

jasnell commented Sep 25, 2019

From what I understand, this was done this way to give a standard/common interface for handle types, regardless of whether the argument is used by the actual implementation. @cjihrig ... if I'm remembering correctly, you were the one who refactored the implementation and structure of these. Do you have an opinion on this?

@cjihrig
Copy link
Contributor

cjihrig commented Sep 25, 2019

if I'm remembering correctly, you were the one who refactored the implementation and structure of these.

I think it was already like that before I refactored the cluster module a few years back, but I could be remembering incorrectly at this point.

From what I understand, this was done this way to give a standard/common interface for handle types, regardless of whether the argument is used by the actual implementation.

That's my understanding as well. At this point in time I don't think it matters much, but in the future, it's possible that cluster is more configurable (something like #11546 maybe), and then it could be more of an issue.

I wouldn't block this from landing, but I'm not enthusiastic enough about it to sign off.

@BridgeAR
Copy link
Member

@MaledongGit this needs a rebase.

@SEWeiTung
Copy link
Author

@BridgeAR : done :)

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

Unfortunately needs another rebase in order to move forward

@SEWeiTung SEWeiTung closed this Jun 19, 2020
@SEWeiTung SEWeiTung deleted the removeUselessParameter branch June 19, 2020 08:31
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants