-
Notifications
You must be signed in to change notification settings - Fork 69
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 API integration to allow TCP ports to be shared #451
Conversation
…en when the cluster size is 1, for simplicity of code.
Not sure if it's related to the udp bug - but I get the following error occasionally when stopping the server:
Maybe you should catch that error? (I couldn't see a |
@@ -14,6 +17,7 @@ config.alerts = config.get('alerts') | |||
config.polling = config.get('polling') | |||
config.reports = config.get('reports') | |||
config.auditing = config.get('auditing') | |||
config.agenda = config.get('agenda') | |||
|
|||
mongoose = require "mongoose" | |||
exports.connectionDefault = connectionDefault = mongoose.createConnection(config.mongo.url) |
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.
Do the mongo connections still need run if the node is master? I believe this will start up a connection pool, so it might save some resources by excluding this
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.
Good point. I'l look at removing it.
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.
Hey @devcritter, it only possible to remove these if we move all /model/*
requires out of the master as these exported connection are used there. I'm not too keen to make that change as its rather large, I don't know if it will affect anything else and I don't think we'll save much by doing so. What do you think?
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.
Aah okay, yeah if it's a mission than definitely skip it. I'm sure it's not too big a deal ito resource usage.
Found an issue when creating new tcp channels from the console - it only starts up the tcp server on one of the workers. When restarting core though, all the workers pick up the tcp channel correctly. Presumably this happens because it's starting a tcp socket on whichever worker gets the API call. A quick solution might be for the API call to just trigger a general restart? |
(the same issue with stopping a TCP channel, e.g. deleting it) |
@rcrichton Just some comments above, but otherwise it works really well! Very cool :) |
Hey, @devcritter I also get that dgram server error some times. I don't think is an issue due to clustering. However, it may be because it isn't possible to wait for the dgram server to close as the function doesn't have a callback (the node API seems a bit different to the norm of the tcp and http API here). I will try to catch this but I'm not quite sure where it's getting thrown, I'm guess at server.close() time. Also, good call on the TCP channels. I'l have to do some more work there. |
…channel is add/updated/removed.
Hey @devcritter, I've addressed the comments. Could you have a look at this again? |
👍 Looks good to me. Excited to see this merged in! :) Just note that the failing tests before merging |
Cool. The test are failing due to a bad mongoose version that I fixed in master this morning. I've merged in master here so they should pass now. I'l wait until they pass before merging this. |
Cluster API integration to allow TCP ports to be shared
OHM-517 Fix tabs on channels modal
Closes #405
This PR allows the OpenHIM to use the cluster API. This will share all servers with the workers, both TCP and HTTP, so, this allows us to cluster even with a single config file.
There is a bug in node preventing me from clustering the UDP sockets (See nodejs/node-v0.x-archive#9261) so those don't get clustered yet. The first worker to get to the server up hosts it.
Use as follows:
node --harmony lib/server/js --cluster=auto
- or replace auto with a number of workers.@devcritter could you please review this?