FirebaseExtended / firebase-queue

MIT License
787 stars 108 forks source link

Change refs #1

Closed m-tse closed 9 years ago

m-tse commented 9 years ago

So, I found it weird that we pass in a reference to the queue subtree of the Firebase, and then we do some weird ref.parent().child('Jobs') in order to access the Jobs sibling. It makes more sense to just pass in a Firebase, and specify that there will be a Jobs Subtree, and a Queue subtree.

@drtriumph since it's a bit ways off before npm release, any objections to simply making this repo public? it will help tremendously with my docker deployment, for me to simply git pull the public repo without worrying about auth.

cbraynor commented 9 years ago

A few comments, back to you @mattse

m-tse commented 9 years ago

@drtriumph proposal:

Instead of having a flag of queue history or no queue history, how about

cbraynor commented 9 years ago

I still like the idea of some kind of 'remove on completion' flag on the job spec, or even special-casing a resolve(null) in the callback code (which is slowly becoming my favorite).

I think once we start moving jobs between subtrees in transactions we start adding a lot of read/write overhead into the system - that editing a single _state value wouldn't incur. The transactions would also have to be at the root of the queue to ensure they are successfully moved instead of on a single item, locking up more of the Firebase.

Chaining jobs also becomes much simpler with a single queue (which is a particular requirement for my needs) - I'm not even sure how we'd set up the job specification to achieve this with multiple queues, and we'd quickly end up with many subtrees with relatively small numbers of chained jobs (2n + 1).

I also like the simplicity of just pushing a new element onto the /queue location as the interface to add a new item - you don't have to worry about the particulars of the queue implementation and can even have all incoming items without any _state variable picked up by the first job

cbraynor commented 9 years ago

For now, if you assign it back to me I'll merge this PR

m-tse commented 9 years ago

@drtriumph I've implemented our discussed changes

cbraynor commented 9 years ago

I had a fair number of coding style and a few algorithmic comments. Happy to explain in more detail if needed

m-tse commented 9 years ago

@drtriumph back to you

cbraynor commented 9 years ago

Alright, we're close! Back to you

m-tse commented 9 years ago

@drtriumph back to you

cbraynor commented 9 years ago

Ok, two tiny comments, back to you @mattse

m-tse commented 9 years ago

@drtriumph okay looks like this is good for a merge? Let's reconvene and discuss next steps.

cbraynor commented 9 years ago

Ok, LGTM