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

Allow custom job ids to be specified in the job options #335

Merged
merged 10 commits into from
Aug 22, 2016
Merged

Allow custom job ids to be specified in the job options #335

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0ef22f8
Allow custom job ids to be specified in the job options
Aug 19, 2016
7c5940c
Treat jobIds as strings
Aug 19, 2016
2e9940b
Delayed jobs should still use a numeric id to offset the timestamp
Aug 19, 2016
02f4009
Don't overwrite if the key already exists
Aug 19, 2016
e744956
Updating readme
Aug 19, 2016
1b765e5
Use an 'if/else' for readability in the 'addJob' script
Aug 22, 2016
77f6b77
Rename 'numericJobId' to 'jobCounter'
Aug 22, 2016
6c8e29d
Renaming 'fullJobId' to 'jobIdKey' in the 'addJob' script
Aug 22, 2016
ceb827f
Changing the order of params to the 'addJob' script
Aug 22, 2016
7369515
Making the readme a bit clearer on the 'jobId' option
Aug 22, 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: 1 addition & 1 deletion lib/job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ var Job = function(queue, data, opts){
function addJob(queue, job){
var jobData = job.toData();
var toKey = _.bind(queue.toKey, queue);
return scripts.addJob(queue.client, toKey, job.opts.lifo, jobData);
return scripts.addJob(queue.client, toKey, job.opts.lifo, job.opts.jobId, jobData);
}

Job.create = function(queue, data, opts){
Expand Down
9 changes: 5 additions & 4 deletions lib/scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ var scripts = {
return result === 1;
});
},
addJob: function(client, toKey, lifo, job){
addJob: function(client, toKey, lifo, customJobId, job){
var jobArgs = _.flatten(_.toPairs(job));
Copy link
Member

@manast manast Aug 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

since lifo is optional and customId also, I think it is more convenient to put it in an opts argument, and also move it to the last argument in the list:function(client, toKey, job, opts)


var keys = _.map(['wait', 'paused', 'meta-paused', 'jobs', 'id', 'delayed'], function(name){
Expand All @@ -74,11 +74,11 @@ var scripts = {
var baseKey = toKey('');

var argvs = _.map(jobArgs, function(arg, index){
return ', ARGV['+(index+2)+']';
return ', ARGV['+(index+3)+']';
})

var script = [
'local jobId = redis.call("INCR", KEYS[5])',
'local jobId = ARGV[2] == "" and redis.call("INCR", KEYS[5]) or ARGV[2]',
'redis.call("HMSET", ARGV[1] .. jobId' + argvs.join('') + ')',
Copy link
Member

Choose a reason for hiding this comment

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

Why not use if/else here for readability ?

];

Expand All @@ -87,7 +87,7 @@ var scripts = {
var delayTimestamp = job.timestamp + job.delay;
if(job.delay && delayTimestamp > Date.now()){
script.push.apply(script, [
' local timestamp = tonumber(ARGV[' + (argvs.length + 2) + ']) * 0x1000 + bit.band(jobId, 0xfff)',
' local timestamp = tonumber(ARGV[' + (argvs.length + 3) + ']) * 0x1000 + bit.band(jobId, 0xfff)',
' redis.call("ZADD", KEYS[6], timestamp, jobId)',
Copy link
Member

Choose a reason for hiding this comment

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

good catch since we use the jobCounter for the delay logic.

' redis.call("PUBLISH", KEYS[6], (timestamp / 0x1000))',
' return jobId',
Expand Down Expand Up @@ -121,6 +121,7 @@ var scripts = {

args.push.apply(args, keys);
args.push(baseKey);
args.push(customJobId || '');
args.push.apply(args, jobArgs);
args.push(delayTimestamp);

Expand Down
7 changes: 7 additions & 0 deletions test/test_job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,13 @@ describe('Job', function(){
expect(storedJob.opts.testOpt).to.be('enabled');
});
});

it('should use the custom jobId if one is provided', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Test case suggestion:

Create a job with a customId and test if it can be processed, in the process callback, verify that the custom jobId matches the one provided

var customJobId = 'customjob';
return Job.create(queue, data, { jobId: customJobId }).then(function(createdJob){
expect(createdJob.jobId).to.be.equal(customJobId);
});
});
});

describe('.remove', function () {
Expand Down