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

v2 work starting #37

Closed grantcarthew closed 7 years ago

grantcarthew commented 7 years ago

Hi everyone (well, the three watching this project).

I am going to start work on v2.0.0 as defined in the project: https://github.com/grantcarthew/node-rethinkdb-job-queue/projects/1

The only change I am not sold on is returning a Promise in the Queue.process function. Everything else sounds good.

grantcarthew commented 7 years ago

@sorgloomer, you will be interested in this.

sorgloomer commented 7 years ago

@grantcarthew Thanks for the notice! Unfortunately I am no longer involved in the project where we use job-queue, so its not vital for me, but let me tell you my thoughts on this:

There are two ways of handling asynchronousity. The old and mature de-facto node-style callback way, and the standard Promise way. People are familiar with these two, I see no reason to use a third one.

But in fact the change is not crucial, when I left the project we were using something like this:

function wrap(processor) {
    return (job, next, onCancel) => {
        next(Promise.resolve().then(() => processor(job, onCancel)));
    };
}

...

q.process(wrap(async (job, onCancel) => {
    const foo = await SomeFirstStep(job);
    return await SomeSecondStep(job, foo);
}))
thomasmodeneis commented 7 years ago

Hi @grantcarthew, First of all, thanks for this great module. 👍

I really like the #34. Good point there.

I've been facing some issues regarding 3 things:

1) The https://github.com/grantcarthew/node-rethinkdb-job-queue/wiki/Queue.summary is taking up to two minutes to return back stats for queues. I'm running now 8 queues and things are getting a bit out of hand, response time is so huge and is causing the entire cluster to crash. So I had to disable the periodic summary update.

2) I'm on ^1.1.4 and I'm facing issues when I create a job but is never picked up, unless I restart the node process. I'm running jobs with changeFeed:trueso it makes me wonder what I'm doing wrong.

3) When I start the producer and the consumer at the same time and the tables are not yet created, I'm ending up having multiple tables with same name, and the entire thing collapses. This is a minor, but maybe worth alerting users about this ?

Also I'm wondering what are your plans for building a web-admin kind of dashboard.

Cheers.

grantcarthew commented 7 years ago

@sorgloomer : Thanks for the feedback. You may be swaying me.

@thomasmodeneis : Interesting feedback, thanks.

1) I don't have as much data in queues as you do. Are you happy to help optimize the query? See #38

2) Do you have a master Queue object defined?

3) I have seen this issue myself once. I don't see anyway this could be mitigated without some sort of random delay on initializing. It is rather helpful having the queue create its own table. Taking that feature out to prevent the issue would seem a shame. Edit: You know what? The random delay might be a good idea. Make it as an option though. Most projects would not be affected by a few extra seconds on boot.

Web-Admin Dashboard

I don't get paid for any of the work I do on this project or my scalable-blob-store. I wanted a queue on RethinkDB so made one as a proof of concept which worked really well.

At this point in time I am a better back-end developer than I am front-end and I am working on my own web application.

I'm going to leave this one up to the community for now.

TomKaltz commented 7 years ago

I have been tinkering on a web dashboard. It's not releasable in it's current state but I hope to contribute it to the community when I feel it's of decent quality.

grantcarthew commented 7 years ago

Oh, good stuff @TomKaltz.

thomasmodeneis commented 7 years ago

1) I'm happy to help sir.

2) I'm not using masterQueue, I just happen to understand what is it and I will update my code in order to make use of it, Thanks for the tip :)

3) Its really helpful having the queue create its own table, this is one of the good things on rethinkdb that I love :) We could make use of the random delay, maybe just to be used when queue need to create the tables/indexes ?

@TomKaltz I could help you out on this one, I was about to build a dashboard this weekend with React/Redux but if you are already doing it then I will keep focusing on other stuff then until you release yours.

Cheers.

grantcarthew commented 7 years ago

I have decided to put in a new Queue option createStoreageRandomDelay or something similar that will be set to false by default. Useful for your scenario where multiple Queues point to the same DB/Table and start at the same time. If you set it to a ms value it will randomly delay the Database/Table/Index assertions within that value.

Good feedback @thomasmodeneis thanks again.

grantcarthew commented 7 years ago

For anyone interested, I am documenting the changes to v2 as I go here: https://github.com/grantcarthew/node-rethinkdb-job-queue/wiki/Change-Log

Currently the testing is taking a lot of my time. There is a bug in tape that has got me checking out tap. Mostly due to this: https://remysharp.com/2016/02/08/testing-tape-vs-tap

grantcarthew commented 7 years ago

It has taken me a while to fix the tests guys however it has been worth it. I have migrated the tests from tape to tap with great success. The tests are a lot more reliable although the output is not as attractive.

Also, thanks to @thomasmodeneis there is now a code coverage report which is visible on the web. See the README under the documentation heading. It is not as high as it should be because I have half implemented the repeat job feature. That said I was very pleasantly surprised to see the full extent of the coverage. Now that the coverage is visible to me I will add tests to bring up the score.

See the change log for more detail.

grantcarthew commented 7 years ago

@thomasmodeneis - I have added a database initialization delay algorithm to fix your third issue raised above. See it here: https://github.com/grantcarthew/node-rethinkdb-job-queue/blob/master/src/db-assert.js

It should prevent the duplicate database name issue.

Thanks again.

grantcarthew commented 7 years ago

Completed. v2.0 is now published on NPM. See the Change Log for details. There are breaking changes.

thomasmodeneis commented 7 years ago

This was a great and effective work @grantcarthew I'm impressed by your skills :) I will update my modules to start using v2.0 and in case of any troubles I will let you know.

Thanks and have a great weekend!

grantcarthew commented 7 years ago

Thanks @thomasmodeneis. I'm hoping to focus on other things for a while. No doubt there will be the odd bug here and there. The feature set of the queue is quite good now. The only thing I have not added is a rate limiter. Was thinking about it however could not come up with a good way of having global limiting.

Thanks again.