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

Create cluster.cr #3672

Closed
wants to merge 5 commits into from
Closed

Conversation

adrien-thierry
Copy link

Add a Cluster-like capacity to Crystal

Add a Cluster-like capacity to Crystal
(ENV["FORKED"] ||= "0") == "1"
end

def self.getEnv (env : String)
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is unneccesary

class Cluster

def self.fork (env : Hash)
env["FORKED"] = "1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would namespace this env var more.

@RX14
Copy link
Contributor

RX14 commented Dec 11, 2016

I would like to see a forking cluster helper in the standard library, however I'm a little dubious on the API. At the very least needs a format and docs.

@adrien-thierry
Copy link
Author

Why not take inspiration from the nodejs Cluster api? https://nodejs.org/docs/latest/api/cluster.html

return Process.fork { Process.run(PROGRAM_NAME, nil, env, true, false, true, true, true, nil ) }
end

def self.isMaster
Copy link
Contributor

Choose a reason for hiding this comment

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

self.master? is more Crystal/Ruby semantic. Same for self.slave?

Copy link
Author

Choose a reason for hiding this comment

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

When I mention the NodeJS Cluster API, I speak about the API, not the semantic

Copy link
Contributor

Choose a reason for hiding this comment

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

NodeJS API adheres to JS conventions, not to Ruby ones. It's a convention to use isFoo in JS, whereas in Ruby you'd rather write foo?.

Copy link
Author

Choose a reason for hiding this comment

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

I understand, changes done

end

def self.isMaster
(ENV["FORKED"] ||= "0") == "0"
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't set value in checker method. Proper way would be (ENV["FORKED"]? || "0") == "0".

Copy link
Author

Choose a reason for hiding this comment

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

Done

Updated semantic
return Process.fork { Process.run(PROGRAM_NAME, nil, env, true, false, true, true, true, nil ) }
end

def self.master
Copy link
Contributor

Choose a reason for hiding this comment

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

def self.master?

@Sija
Copy link
Contributor

Sija commented Dec 11, 2016

@adrien-thierry NodeJS cluster comes with more than just wrapping a child process with ENV parameter. Quoting from the docs:

A single instance of Node.js runs in a single thread. To take advantage of multi-core systems the user will sometimes want to launch a cluster of Node.js processes to handle the load.

The cluster module allows you to easily create child processes that all share server ports.

The worker processes are spawned using the child_process.fork() method, so that they can communicate with the parent via IPC and pass server handles back and forth.

That said, having similar capabilities in Crystal would be great but IMHO this PR in its current form doesn't add much value to stdlib.


def self.fork (env : Hash)
env["FORKED"] = "1"
return Process.fork { Process.run(PROGRAM_NAME, nil, env, true, false, true, true, true, nil ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant return

change "slave" by "worker"
@adrien-thierry
Copy link
Author

@Sija With SO_REUSEPORT support, it's a goot start to add easy multithreading-like capabilities to http server. As is, it lacks events (close etc) and IPC

Removed redundant return
@bcardiff
Copy link
Member

It's better to see something like this evolve as a separate shard first.

I would suggest also to take a look at https://github.com/ysbaddaden/panzer .

Thanks!

@bcardiff bcardiff closed this Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants