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

Deletes old jobs, addresses search term race conditions, and adds "indexing" #218

Closed
wants to merge 11 commits into from

Conversation

aventuralabs
Copy link

Three proposed fixes in this pull request:

  1. Solves race conditions inherent in indexing. As discussed in search keys not being removed #94 and jobs removed from event Job map prematurely when attempts > 1  #98, jobs may incorrectly be removed before search indexes have completed. I add an "is_indexed" parameter and force that to be true for removal. If it is not true, the remove function listens for the event via pubsub and processes once is_indexed has been called.
  2. Adds "indexing" capabilities as discussed in Proposition to reduce search key explosion #217. Namely, auto-indexing all fields does not make sense for a job queue. Job constructor now includes third parameter indexes. When undefined, operates traditionally (for backwards-compatibility), a blank string indexes only the job type, otherwise, indexes only specified keys. Keys are separated by spaces. For example 'key3 key21 key33'.
  3. Removes old jobs based on type and expiration (addresses Remove old jobs? #58).
var kue = require('kue'),
   , jobs = kue.createQueue();

// Creates a job with indexes for fname and lname
var jobOptions = { indexes : 'fname lname', expires : true };
var job = jobs.create('test-job', { uId : '12345671', fname : 'flying', lname : 'squirrel'}, jobOptions);

// Sets an expiration on test-jobs (5 seconds)
jobs.setExpiration('test-job', 5000);

job.save(function(err) {
  // Finds the job based on indexed terms only
  jobs.find('test-job flying', function(err, ids) { console.log(ids) });  
});

// After 5 seconds, item is auto-removed, including search terms
jobs.on('job complete', function(id) {
  setTimeout(function() {
    kue.Job.get(id, function(err, job) {
      // Job will already have been deleted
    });
  }, 6000);
});

Furthermore, the state of Redis will look like this (assuming no other jobs):

redis 127.0.0.1:6379> KEYS *
1) "q:stats:work-time"
2) "q:ids"
3) "q:job:types"

Finally, I don't have access to the testing suite, but tried to run it through a number of scenarios ahead of submitting the pull request.

Brandon

@drudge
Copy link
Contributor

drudge commented Jul 16, 2013

This looks like a pretty cool commit, I want to test it out before I merge though. Do you have any of your tests handy?

@aventuralabs
Copy link
Author

Thanks!

Here's a gist with some fairly crappy, unstructured tests (I haven't been able to do a formal test suite yet).

https://gist.github.com/brandoncarl/6011608

I've been using it in my application and debugging little bits as you can see from commit history. Fingers crossed, it seems decently stable. I thought maybe you could run it against existing test suite if you have one...I created it with backwards compatibility in mind.

@behrad
Copy link
Collaborator

behrad commented Oct 19, 2013

It doesn't seem to work for me! I merged it with my own for from the 0.6.2! I only replaced the kue dependency, not the default client code using it!

@wachunga
Copy link

wachunga commented Feb 3, 2014

@behrad in the latest kue release, has this been addressed somehow independent of this pull request? How else are you handling millions of jobs without some kind of expiry?

My team would like to update to the latest kue but we need some kind of job expiry in production.

@behrad
Copy link
Collaborator

behrad commented Feb 3, 2014

We have written a watchdog script whose job is to delete completed/failed jobs living more than n minutes @wachunga
We were not using search indexes (they are off), I have not traffic tested with search indexes on, but home tests show not indexes remaining in redis after some changes in remove in 0.7.0

@behrad
Copy link
Collaborator

behrad commented Apr 27, 2014

please check #94 & #284 & #282

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.

None yet

5 participants