- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 33.6k
doc: improvements to cluster.markdown copy #4409
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
Conversation
        
          
                doc/api/cluster.markdown
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"terminated normally" or just "exited" would be more consistent with child process docs
| @sam-github ... updated to address #4409 (comment) | 
837b9a7    to
    938d867      
    Compare
  
            
          
                doc/api/cluster.markdown
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first sentence is a bit long in the tooth and hard to understand. Maybe the same thing can be said with less.
"Node.js executes JavaScript code within the context of a single-threaded event loop."
| @ryansobol ... fixed! | 
        
          
                doc/api/cluster.markdown
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a stab at making the cluster introduction more approachable to beginners. I was able to consolidate three paragraphs down to two which is a win with respects to the reader quickly seeing their first code example. In addition, I tried to answer why clusters are important by first explaining the benefits and shortcomings of libuv concurrency model at a high level.
In the process, the references about child_process.fork() and IPC channels was cut. In my experience as a teacher, a newcomer wants to know three things in the beginning—what a thing is, why it's important, and how to use it. Implementation details can come later.
Anyway, feel free to use none, some, or all of it. :)
Edit: Remembered that the cluster API's primarily used for non-blocking socket I/O bound apps. While I'm sure it's also useful for CPU bound apps too, it's probably better to focus on the primary use case.
Node.js executes JavaScript code within the context of an event loop. Under the hood, non-blocking socket I/O runs on the main event loop thread while blocking DNS and filesystem I/O runs on a secondary thread that notifies the main thread when it's finished. This concurrency model allows a blocking I/O bound application to easily distribute work across multiple CPU cores. For more details, see the libuv project.
The cluster API allows a non-blocking I/O bound application to distribute its work across multiple CPU cores. A Node.js cluster is created when a master process forks one or more worker processes. By design, workers use the same application code as the master. Conveniently, the cluster.isMaster property determines if a running process is the master or a worker.
const cluster = require('cluster');
if (cluster.isMaster) {
  for (var i = 0; i < numCPUs; i++) {
    cluster.fork();
  }
} else {
  // Non-blocking I/O work
}
959477e    to
    b075d9c      
    Compare
  
    | Updated to use @ryansobol's wording for the intro | 
9b87870    to
    6fb5ac7      
    Compare
  
    | @nodejs/documentation @sam-github @ryansobol ... would appreciate one more quick review on this before landing. | 
        
          
                doc/api/cluster.markdown
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add const numCPUs = require('os').cpus().length;?
| @ryansobol ... thanks for the comments! I'll update shortly | 
Per nodejs#4409, the documentation on http.abort is a bit lacking. This provides a slight improvement. Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs/node-v0.x-archive#25591
| Hey @jasnell are you still working on these? Need a review? | 
| @benjamingr ... yeah, I still have a few of these queued up. I plan to revisit them hopefully later this week. | 
A number of general improvements to cluster.markdown including copyedits and revised examples
6fb5ac7    to
    aa74290      
    Compare
  
    | @benjamingr @ryansobol ... ping.. rebased and updated! | 
| ```js | ||
| const cluster = require('cluster'); | ||
| if (cluster.isMaster) { | ||
| for (var i = 0; i < numCPUs; i++) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally prefer let in loops but I see both var or let in the docs. (https://github.com/nodejs/node/blob/master/doc/api/buffer.markdown has both)
Excuse me if this has been brought up before.
7da4fd4    to
    c7066fb      
    Compare
  
    | So this is not going to land? | 
Per nodejs#4409, the documentation on http.abort is a bit lacking. This provides a slight improvement. Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs/node-v0.x-archive#25565
A number of general improvements to cluster.markdown including
copyedits and revised examples