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.

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

Rewrite bee-queue 1.0.0 #64

Merged
merged 84 commits into from Aug 9, 2017

Conversation

@skeggse
Copy link
Member

skeggse commented Jul 13, 2017

Note that we haven't re-attained 100% code coverage, and that's the biggest blocker to considering this ready to merge.

@skeggse skeggse requested a review from LewisJEllis Jul 13, 2017
@skeggse skeggse force-pushed the mixmaxhq:master branch from ffce676 to f61ae2c Jul 13, 2017
@@ -1,14 +1,29 @@
0.3.0 / 09-01-2015
1.0.0 / 2017-06-30

This comment has been minimized.

Copy link
@bradvogel

bradvogel Jul 13, 2017

Contributor

these new features need to be documented in the readme

"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"

This comment has been minimized.

Copy link
@bradvogel

bradvogel Jul 13, 2017

Contributor

bee-queue/bee-queue

This comment has been minimized.

Copy link
@bradvogel

bradvogel Jul 13, 2017

Contributor

same with other places in this file

This comment has been minimized.

Copy link
@skeggse

skeggse Jul 13, 2017

Author Member

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) {

This comment has been minimized.

Copy link
@bradvogel

bradvogel Jul 13, 2017

Contributor

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",

This comment has been minimized.

Copy link
@LewisJEllis

LewisJEllis Jul 13, 2017

Member

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.

This comment has been minimized.

Copy link
@skeggse

skeggse Jul 13, 2017

Author Member

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 skeggse force-pushed the mixmaxhq:master branch 2 times, most recently from 5a1a305 to e7aee81 Jul 13, 2017
@LewisJEllis

This comment has been minimized.

Copy link
Member

LewisJEllis commented Jul 13, 2017

My 10-minute skim impression/thoughts:

  • Looks great overall, you've kept along a lot of the original ideas/tenets I had when starting this and polished it to be 2017 JS instead of 2015 JS. Most of the spots where the 2-years-later version of me sees room for stuff to be better, it is, especially around tooling/CI/etc.
  • Good move on the promise transition. I skipped them previously to dodge bluebird/a compilation step/node 0.12's sketchy promise situation, but now that they're a standard language feature (along with async/await) I'm happy to see/use them. Curious about your thoughts on the option of moving to an entirely promise-first API and maybe obviating the need for promise-callback?
  • I'm gonna have to go through in more detail to understand the new timer stuff and the new behavior for stalling/delayed jobs; definitely adds complexity, but it looks well-thought-out.

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 :)

@LewisJEllis

This comment has been minimized.

Copy link
Member

LewisJEllis commented Jul 13, 2017

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...).

@bradvogel

This comment has been minimized.

Copy link
Contributor

bradvogel commented Jul 17, 2017

@LewisJEllis fyi: we're still debugging one last issue with this PR (running this in production!). So wait on reviewing.

@skeggse skeggse force-pushed the mixmaxhq:master branch from 466a606 to ab0ba56 Jul 19, 2017
@skeggse

This comment has been minimized.

Copy link
Member Author

skeggse commented Jul 19, 2017

@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?

@LewisJEllis

This comment has been minimized.

Copy link
Member

LewisJEllis commented Jul 19, 2017

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.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 19, 2017

Coverage Status

Changes Unknown when pulling 92535f4 on mixmaxhq:master into ** on bee-queue:master**.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 20, 2017

Coverage Status

Changes Unknown when pulling 0db0f76 on mixmaxhq:master into ** on bee-queue:master**.

@skeggse

This comment has been minimized.

Copy link
Member Author

skeggse commented Jul 20, 2017

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 Promise implementation at runtime.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 20, 2017

Coverage Status

Changes Unknown when pulling fad6899 on mixmaxhq:master into ** on bee-queue:master**.

skeggse added 10 commits Aug 8, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Changes Unknown when pulling 4574ac0 on mixmaxhq:master into ** on bee-queue:master**.

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

This comment has been minimized.

Copy link
@LewisJEllis

LewisJEllis Aug 8, 2017

Member

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.

This comment has been minimized.

Copy link
@skeggse

skeggse Aug 8, 2017

Author Member

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.

This comment has been minimized.

Copy link
@LewisJEllis

LewisJEllis Aug 8, 2017

Member

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.

// down during this operation.
return this._commandable().then((client) => {
const got = helpers.deferred();
const commandArgs = [this.toKey('jobs')].concat(idsArray, got.defer());

This comment has been minimized.

Copy link
@LewisJEllis

LewisJEllis Aug 8, 2017

Member

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?

This comment has been minimized.

Copy link
@LewisJEllis

LewisJEllis Aug 8, 2017

Member

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.

This comment has been minimized.

Copy link
@LewisJEllis

LewisJEllis Aug 8, 2017

Member

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.

This comment has been minimized.

Copy link
@skeggse

skeggse Aug 8, 2017

Author Member

Related. I'm adding this functionality, but it won't be used in Arena for them time being.

skeggse added 3 commits Aug 8, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Changes Unknown when pulling 66d8cf0 on mixmaxhq:master into ** on bee-queue:master**.

4 similar comments
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Changes Unknown when pulling 66d8cf0 on mixmaxhq:master into ** on bee-queue:master**.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Changes Unknown when pulling 66d8cf0 on mixmaxhq:master into ** on bee-queue:master**.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Changes Unknown when pulling 66d8cf0 on mixmaxhq:master into ** on bee-queue:master**.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Changes Unknown when pulling 66d8cf0 on mixmaxhq:master into ** on bee-queue:master**.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Changes Unknown when pulling 936b9b6 on mixmaxhq:master into ** on bee-queue:master**.

Copy link
Member

LewisJEllis left a comment

Looks ready!

@skeggse skeggse merged commit ffe5194 into bee-queue:master Aug 9, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@skeggse

This comment has been minimized.

Copy link
Member Author

skeggse commented Aug 9, 2017

🎉

@LewisJEllis

This comment has been minimized.

Copy link
Member

LewisJEllis commented Aug 9, 2017

Added a .npmignore in a8e7183 and published!

@skeggse

This comment has been minimized.

Copy link
Member Author

skeggse commented Aug 9, 2017

The other option there, @LewisJEllis, is to specify a whitelist via the files array in the package.json.

@LewisJEllis

This comment has been minimized.

Copy link
Member

LewisJEllis commented Aug 9, 2017

Oh, nice, TIL. That seems way easier given that I started with a whitelist in the comments of the .npmignore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.