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

Add improvements to run kue from a worker process #579

Closed
wants to merge 3 commits into from

Conversation

jesucarr
Copy link

See issue #574 for more details.

The shutdown doesn't close the process, but it is unrelated to the feature added. I know it is still work in progress so I guess it will be fixed by the time it is released.

@behrad
Copy link
Collaborator

behrad commented Apr 15, 2015

shutdown should close the process when disableSearch is true

@jesucarr
Copy link
Author

If you look at the worker examples, disableSearch is true, but doesn't close the process, and anyway it should be true by default as far as I understand...

I guess this is still a work in progress on your side, since it is unrelated to the code I added.

@behrad
Copy link
Collaborator

behrad commented Apr 16, 2015

I remember we had a similiar bug which had been fixed, but when search indexes are enabled, the reds module is opening new connections to redis and Kue is not able to close them :( Search indexes are turned off by default in 0.9 #412

Would you break down this PR into two:

  1. only the changes to kue.js as a promotion event addition,
  2. a failing test case for the shutdown problem added to tests

@jesucarr
Copy link
Author

The PR I sent is exactly your point 1)

Regarding 2) I'm not sure how to create that test case. It seems that the redis client is ended, but the process doesn't exit

@behrad behrad added the ready label Apr 17, 2015
queue.on('job complete', function(result) {
console.log('Job completed with result', result);
closeIfCan();
}).on('promotion:end', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You cannot depend on promotion:end being called in every node in a clustered deployment.
Should we add a mock promotion:end event to all nodes that are silent (losers to lock race)? or re-view the whole stuff again?

@jesucarr
Copy link
Author

I had to go ahead with another alternative, and I couldn't have a deep look on how this really works. So yeah feel free to close this PR and review the whole stuff again

@behrad
Copy link
Collaborator

behrad commented Apr 21, 2015

then closing this for now :)

@behrad behrad closed this Apr 21, 2015
@behrad behrad removed the ready label Apr 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants