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.

Sign up for GitHub

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

Jump to bottom

Add new 'removeOnComplete' option to atomically remove a job when it … #361

Merged
merged 3 commits into from
Oct 17, 2016
Merged

Add new 'removeOnComplete' option to atomically remove a job when it … #361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c8e8cee
Add new 'removeOnComplete' option to atomically remove a job when it …
bradvogel Oct 15, 2016
4debba5
Micro-optimization: Since we can assume there's only one job id in th…
bradvogel Oct 15, 2016
a0420e3
Give script different name so it doesn't get cached with considering …
bradvogel Oct 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,8 @@ __Arguments__
If you use this option, it is up to you to ensure the
jobId is unique. If you attempt to add a job with an id that
already exists, it will not be added.
removeOnComplete {Boolean} A boolean which, if true, removes the job when it successfully
completes. Default behavior is to keep the job in the completed queue.
}
returns {Promise} A promise that resolves when the job has been succesfully
added to the queue (or rejects if some error occured). On success, the promise
Expand Down
2 changes: 1 addition & 1 deletion lib/job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ Job.prototype.delayIfNeeded = function(){

Job.prototype.moveToCompleted = function(returnValue, token){
this.returnvalue = returnValue || 0;
return scripts.moveToCompleted(this, token || 0);
return scripts.moveToCompleted(this, token || 0, this.opts.removeOnComplete);
};

Job.prototype.moveToFailed = function(err){
Expand Down
29 changes: 16 additions & 13 deletions lib/scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ var scripts = {

return execScript.apply(scripts, args);
},
moveToCompleted: function(job, token){
moveToCompleted: function(job, token, removeOnComplete){
var keys = _.map([
'active',
'completed',
Expand All @@ -143,20 +143,23 @@ var scripts = {
return job.queue.toKey(name);
}
);

keys.push(job.lockKey());

//
// INVESTIGATE: Should'nt we check if we have the lock before trying to move the
// job?
//
var deleteJob = 'redis.call("DEL", KEYS[3])';
var moveJob = [
'redis.call("SADD", KEYS[2], ARGV[1])',
'redis.call("HSET", KEYS[3], "returnvalue", ARGV[2])',
].join('\n');

var script = [
'if redis.call("EXISTS", KEYS[3]) == 1 then',
' redis.call("SADD", KEYS[2], ARGV[1])',
' redis.call("HSET", KEYS[3], "returnvalue", ARGV[2])',
' redis.call("LREM", KEYS[1], 0, ARGV[1])',
' if redis.call("get", KEYS[4]) == ARGV[3] then', // Remove the lock
' return redis.call("del", KEYS[4])',
'if redis.call("EXISTS", KEYS[3]) == 1 then', // Make sure job exists
' if redis.call("GET", KEYS[4]) == ARGV[3] then', // Makes sure we own the lock
' redis.call("LREM", KEYS[1], -1, ARGV[1])',
removeOnComplete ? deleteJob : moveJob,
' redis.call("DEL", KEYS[4])',
' return 0',
' else',
' return -1',
' end',
Copy link
Member

@manast manast Oct 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

A slight issue here, you need to name differently the script when you specify removeOnComplete, otherwise, since scripts are cached, if you do not use removeOnComplete in the future in the same instance, it will use the old cached one. I know this is probably not going to happen, but for the sake of formality :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, great catch. Pushed up a fix. Also pushed a small microoptimization for queues with a large number of active jobs (like ours)

'else',
' return -1',
Expand All @@ -165,7 +168,7 @@ var scripts = {

var args = [
job.queue.client,
'moveToCompletedSet',
'moveToCompletedSet' + (removeOnComplete ? 'RemoveOnComplete' : ''),
script,
keys.length,
keys[0],
Expand Down