grantcarthew / node-rethinkdb-job-queue

A persistent job or task queue backed by RethinkDB.
https://github.com/grantcarthew/node-rethinkdb-job-queue/wiki
MIT License
156 stars 16 forks source link

only `next` allows to set `data` property of job log #17

Closed marcoalbarelli closed 8 years ago

marcoalbarelli commented 8 years ago

Don't kow if this was an intended behaviour, but right now the only way to cleanly set the data property of a log entry is via the next callback in the process function

It should be possible to set it also un the update method of the job

grantcarthew commented 8 years ago

You can simply call job.createLog() and then set any values on the log object.


let log = job.createLog()
log.message = 'this is a log message'
log.data = 'anything'

Your PR will add another options though.

marcoalbarelli commented 8 years ago

What you say is partially true as far as I understood: The behaviour I need is to update the job and mark it as completed, settind the data in the generated log entry. Right now I can do almost the same thing, but I can only fill the message property. I could live with that of course but populating the data porperty of the log entry is what the next in the process does so I guess it's not a BadThing™ :)

If I overlooked a simple way to obtain this (completed job with data in the completion log entry) please let me know

grantcarthew commented 8 years ago

The behaviour I need is to update the job and mark it as completed, settind the data in the generated log entry.

Is this from within the next function or somewhere else?

To clear confusion I'll go through the current methods for logging against jobs.

next(result) with Successful Job

Will result in the following:

log.message = 'Job completed successfully' <= No control over this message log.data = result <= result can be a value or object

next(err) with Failed Job

Will result in the following:

log.message = 'Job processing failed' <= No control over this message log.errorMessage = err.message <= The err message you pass to next log.errorStack = err.stack <= The err stack trace

Job.createLog(message, type, status)

You have a PR on this, however you can do the following:


let log = job.createLog()
log.message = 'something'
log.data = { foo: 'bar' }
log.anything = 'whatever'

job.addLog(log)

Job.addLog(value)

In this case value can be a Log, string, or object.

If value is a string it will be assigned to log.message. If value is an object it will be assigned to log.data.

Custom Job Changes with Job.update()

You can also do the following at any time with the Job object:


let log = job.createLog()
// customize the log any way you like.

job.log.push(log)
job.update().catch(err => console.error(err))

So which one of the above is not doing the job for you?

grantcarthew commented 8 years ago

@marcoalbarelli Can you comment on above please?

marcoalbarelli commented 8 years ago

After reading your comment I think the update() method would have done what I needed. On a larger perspective I think there are some methods that don't respect the SRP and that may benefit from some uniformation on a conceptual level.

Basically at every step of the way I'd like to be able to add a log entry with a data property without silently marking the job as completed or whatever else state. This includes the process method of the queue If next(err) makes a lot of sense, marking the job as completed if I call next('somecontent') does not. In the latter case I'd like to be able to specify in what state I want the job to be when I'm done with it.

Had this been possible I wouldn't have had to create a separate queue in my current project

Regarding my PR, you're right in saying that I could have achieved my goal using addLog() ... update() I was mislead by the method names: I found a createLog that also adds the log, so I didn't look any further (kind of in a hurry right now)

All in all I think we only need some minor cleaning up in method names (or better documentation for the lazy guys like me) This and possibly the change of signature of the process()...next() method, but I'm not really sure about the possible side effects of changing that

grantcarthew commented 8 years ago

OK, I think I am understanding your issue now.

I agree the changes to the job.log API are not complete. I think I will get rid of Job.createLog.

In my opinion @marcoalbarelli, I think you are trying to put your application logic into a simple building block. This package is designed to make tasks persistent, reliable, and distributed.

As you have stated in #16, the correct approach is to break your application logic down into individual tasks that need persistent, reliable, distributed processing and build a queue for each one of these. Then at the completion of each subtask, manage the higher level logic in your higher level application modules.

As an example where multiple tasks need to be performed, if someone was registering for your web application and you needed to a) send a confirmation email, and upon activation from said email, b) register the account with a third party service, you would need a data store and two queues to achieve this.

The logic would go something like this;

  1. User registers
  2. Your application saves the registration to your application database (not the job queue).
  3. In the same code that you save the registration to your database, initiate a job within the Q1 to send an activation (confirmation) email.
  4. At the completion of the job, update your registration in your application database.
  5. Wait for the user to click the activation link.
  6. After the user activates the new account, update your registration in your application database.
  7. In the same code that you update the registration, initiate a job within the Q2 to register the new account with the third party service.
  8. After the job completes, update your registration to a completed state in your application database.
  9. Possibly as a last task, initiate a job in Q3 to send a congratulations email or something similar.

In the steps above, the state of the high level task is managed in your application database, not in the job queue.

I'm going to close this now however I will open a new issue to remove the Job.createLog method.

Thanks for your help @marcoalbarelli.

@tomkaltz, you may be interested in this.