Rewrite bee-queue 1.0.0 #64
Conversation
| @@ -1,14 +1,29 @@ | |||
| 0.3.0 / 09-01-2015 | |||
| 1.0.0 / 2017-06-30 | |||
There was a problem hiding this comment.
these new features need to be documented in the readme
package.json
Outdated
| "author": "Lewis J Ellis <me@lewisjellis.com>", | ||
| "license": "MIT", | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/LewisJEllis/bee-queue.git" | ||
| "url": "https://github.com/mixmaxhq/bee-queue.git" |
There was a problem hiding this comment.
same with other places in this file
There was a problem hiding this comment.
Oops, thought I got these all. Done.
lib/job.js
Outdated
| toKey('id'), toKey('jobs'), toKey('delayed'), toKey('earlierDelayed'), | ||
| this.id || '', this.toData(), this.options.delay); | ||
|
|
||
| if (this.queue.settings.processDelayed) { |
There was a problem hiding this comment.
setting should be documented in readme
| "description": "A simple, fast, robust job/task queue, backed by Redis.", | ||
| "main": "index.js", | ||
| "dependencies": { | ||
| "redis": "1.0.0" | ||
| "promise-callbacks": "^3.0.0", |
There was a problem hiding this comment.
I'd much prefer if we pin these. I'm guessing it's just oversight, but I'm happy to discuss if you'd explicitly rather not.
There was a problem hiding this comment.
We prefer to not pin dependencies, because it seems more the job of the application to ensure that its dependencies are locked. In our case, we use yarn, but other folks may use npm shrinkwrap or lockfiles. For us, not pinning dependencies is beneficial because yarn deduplicates common dependencies.
skeggse
force-pushed
the
master
branch
2 times, most recently
from
5a1a305
to
e7aee81
Compare
|
My 10-minute skim impression/thoughts:
It sounds like you're still working on tests, which do make review easier, so I'm inclined to wait on those, but just let me know whenever you think they're in good shape for review. Maybe that's even now, I'm not sure - happy to review now during WIP or wait, just let me know either way. Just hoping to avoid leaving comments of "there should be a test for this" when you already know :) |
|
Oh, also definitely looking for more readme updating, especially if you want to go waving a flag of "hey we're updated and back in action". I'd guess my initial release time breakdown was 1:1:1 impl/tests/readme, and I think that was a significant factor to why this initially caught some adoption (well, that and the whim of the editor of the Node Weekly newsletter...). |
|
@LewisJEllis fyi: we're still debugging one last issue with this PR (running this in production!). So wait on reviewing. |
|
@LewisJEllis it looks like maybe Coveralls isn't functioning now that bee-queue moved to its own organization. I'm not able to get Coveralls to discover bee-queue via my account, can you take a look? |
|
Sure thing - I went into my coveralls account and hit some sort of button that turned https://coveralls.io/github/lewisjellis/bee-queue into https://coveralls.io/github/bee-queue/bee-queue and synced/updated everything; I think it should try to do a report on the next push, so give it a try. If that doesn't work, you also might try moving to a new PR that's on a branch in this repo rather than on the mixmaxhq fork. |
|
Changes Unknown when pulling 92535f4 on mixmaxhq:master into ** on bee-queue:master**. |
|
Changes Unknown when pulling 0db0f76 on mixmaxhq:master into ** on bee-queue:master**. |
|
We're running into what may be a performance regression Node-side (Redis is still performing quite nicely) - I suspect that the switch to promises, especially ES6 promises, may be creating substantially more objects than we expected. I'm seeing GC overhead of up to 15% in production. Thoughts about performance and promises, @LewisJEllis? It looks like maybe bluebird is more optimized relative to es6 promises, so maybe we could support overriding the |
|
Changes Unknown when pulling fad6899 on mixmaxhq:master into ** on bee-queue:master**. |
|
Changes Unknown when pulling 4574ac0 on mixmaxhq:master into ** on bee-queue:master**. |
lib/queue.js
Outdated
| this._delayedTimer = new EagerTimer(this.settings.nearTermWindow); | ||
| this._delayedTimer.on('trigger', this._activateDelayed.bind(this)); | ||
| // To avoid changing the hidden class of the Queue. | ||
| this._delayedTimer = this.settings.activateDelayedJobs |
There was a problem hiding this comment.
I think this could just be
if (this.settings.activateDelayedJobs) {
this._delayedTimer = new EagerTimer(this.settings.nearTermWindow);
this._delayedTimer.on('trigger', this._activateDelayed.bind(this));
}The if (this._delayedTimer) { check will still work the same way with _delayedTimer just being undefined instead of null.
There was a problem hiding this comment.
Yeah, it'd work. I prefer to keep properties on objects in all cases, because doing otherwise introduces a different hidden class. I don't expect that to really have any performance benefits. I also think it's nice to always have the full set of properties always present on an object, so you can see them easily in the REPL.
There was a problem hiding this comment.
Yea, I'm also doubtful it would have any performance benefit since it's just 1 vs 2, not 1 vs 20 or log(n) or n - but the "always see all properties in REPL" bit is a fair point that I hadn't considered. Sounds good as is.
lib/queue.js
Outdated
| // down during this operation. | ||
| return this._commandable().then((client) => { | ||
| const got = helpers.deferred(); | ||
| const commandArgs = [this.toKey('jobs')].concat(idsArray, got.defer()); |
There was a problem hiding this comment.
In the previous getJobs implementation, by deferring to getJob and fromId we would just use the job instance from the local jobs map if available; here it seems we fetch all jobs from redis and make new instances. Could we continue to use the local instances that are already available and only hmget the missing ones?
There was a problem hiding this comment.
My specific thinking here is that this could have some benefit in the use case of "arena is sitting here calling getJobs every 10 seconds to keep an up-to-date dashboard display of jobs that are waiting/failed/etc". I have no idea if arena in particular actually does something like that or if the intent is to use it like that, but it seems like a reasonable monitoring possibility.
There was a problem hiding this comment.
Although on second thought, maybe in that case we would want to have disabled storeJobs on that queue instance anyway to avoid memory issues, and so this wouldn't make any difference. I guess the "watch jobs that have failed" use case wouldn't be so sensitive to it assuming we don't end up with many failed jobs, but for the "watch jobs that are waiting" use case we would almost certainly want to have storeJobs disabled.
So I think my current take is "probably doesn't actually matter, fine as is", but curious what you think. Also curious what @randallm thinks from the arena usage perspective.
There was a problem hiding this comment.
Related. I'm adding this functionality, but it won't be used in Arena for them time being.
|
Changes Unknown when pulling 66d8cf0 on mixmaxhq:master into ** on bee-queue:master**. |
4 similar comments
|
Changes Unknown when pulling 66d8cf0 on mixmaxhq:master into ** on bee-queue:master**. |
|
Changes Unknown when pulling 66d8cf0 on mixmaxhq:master into ** on bee-queue:master**. |
|
Changes Unknown when pulling 66d8cf0 on mixmaxhq:master into ** on bee-queue:master**. |
|
Changes Unknown when pulling 66d8cf0 on mixmaxhq:master into ** on bee-queue:master**. |
|
Changes Unknown when pulling 936b9b6 on mixmaxhq:master into ** on bee-queue:master**. |
|
🎉 |
|
Added a .npmignore in a8e7183 and published! |
|
The other option there, @LewisJEllis, is to specify a whitelist via the |
|
Oh, nice, TIL. That seems way easier given that I started with a whitelist in the comments of the .npmignore. |
Note that we haven't re-attained 100% code coverage, and that's the biggest blocker to considering this ready to merge.start,endparameters for_getJobs