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.

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

add: scheduler config option for cluster #42308

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add: scheduler config option for cluster #42308

wants to merge 1 commit into from

Conversation

yashLadha
Copy link
Contributor

@yashLadha yashLadha commented Mar 12, 2022 •
edited
Loading

This is a WIP PR for adding custom scheduler to cluster.

Will add the doc changes later after the finalisation of structure in PR.

@nodejs-github-bot nodejs-github-bot added cluster Issues and PRs related to the cluster subsystem. needs-ci PRs that need a full CI run. labels Mar 12, 2022
@yashLadha yashLadha added the needs-benchmark-ci PR that need a benchmark CI run. label Mar 12, 2022
@yashLadha yashLadha force-pushed the add_customer_scheduler branch 5 times, most recently from cbe6731 to 21d893d Compare March 13, 2022 10:20
@nodejs-github-bot

This comment was marked as duplicate.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Mar 14, 2022
Open
Open
Open
Open
@yashLadha
Copy link
Contributor Author

yashLadha commented Apr 1, 2022 •
edited
Loading

Opening this for initial review. Will add the docs later after the finalisation.

@yashLadha yashLadha marked this pull request as ready for review April 1, 2022 02:02
jasnell
jasnell reviewed Apr 3, 2022
View reviewed changes
lib/internal/cluster/primary.js
function validateAndReturnScheduler(scheduler, schedulingPolicy) {
if (scheduler !== undefined) {
if (typeof scheduler.execute !== 'function') {
throw new ERR_CLUSTER_INVALID_SCHEDULER('scheduler.execute');
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this warrants a new error code. ERR_INVALID_ARG_TYPE would probably still work here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

INVALID_ARG_TYPE seems very broad and that's why i made the choice of ERR_CLUSTER_INVALID_SCHEDULER. Will update the use to INVALID_ARG_TYPE.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 10, 2022

It's nice to see this picked back up. Just a word of caution @yashLadha - I don't know how popular cluster is in userland anymore, but it is very sparsely maintained in core. When people inevitably have problems with their custom schedulers, you're going to be the main person asked to help debug it. Are you sure you want to sign up for that 😄 (that's why I originally decided not to pursue this)?

@yashLadha
Copy link
Contributor Author

@cjihrig Sure, reason why i wanted this being i also faced similar issue at one the projects and though if this is part of core cluster then that would be awesome.

@yashLadha
cluster: scheduler config option for cluster
5cbeb35 
scheduler let's use a custom scheduler function for scheduling workers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

Assignees
No one assigned
Labels
cluster Issues and PRs related to the cluster subsystem. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yashLadha @nodejs-github-bot @cjihrig @jasnell