FirebaseExtended / firebase-queue

MIT License
786 stars 108 forks source link

[Feature] Add/Remove workers dynamically. #71

Closed startswithaj closed 8 years ago

startswithaj commented 8 years ago

Description

I'm writing another project that monitors/scales firebase queues using wait-time and process-time and available system resources.

The way I currently have this working is to treat Queues as singleton workers and pass a factory method to my monitor/scaler which creates another Queue when it needs to add another worker. This is not logically optimal and adds unnecessary overhead (extra connections to firebase for the task and spec listeners). I could ofcourse simulataneous shutdown the current Queue and start another with my desired numWorkers param. This again seems unnecessary and a little dirty.

I think it would be better for me (and maybe others) if I could add/reduce the number of workers without having to A. create another Queue, B. Shutdown the current Queue start it again with new numWorkers parameter.

Huge N.B. caveat: I don't fully understand the QueueWorker.prototype.setTaskSpec and QueueWorker.prototype._setUpTimeouts methods.

If @mbleigh or @drtriumph have time to look over this be much appreciated.

Code sample

var q = new Queue(th.testRef, _.noop);
q.getWorkerCount()
q.addWorker()
q.removeWorker()

Linter and test suite

Linting and tests passing

Added tests

I added several tests. To cover the bulk of my changes. The outstanding case is where a worker is added to a queue with a specified specID and before that spec has been retrieved from firebase. This is accounted for by not covered by tests.

I tried to write this with as few changes to the existing code. It would be good if things like creating the new queueWorker were refactored out and there weren't multiple for loops for setting the task specs of the workers.

Sign our CLA

Signed

startswithaj commented 8 years ago

bump

abeisgoat commented 8 years ago

cc @drtriumph if you haven't seen this already

cbraynor commented 8 years ago

Alright, first pass through this. It looks fairly simple and I'd be happy to merge it in if you address all the issues. Thanks for the taking the time to share

startswithaj commented 8 years ago

@drtriumph thanks for looking at this for me. With my latest 2 commits I think I have addressed all the stuff you listed above:

cbraynor commented 8 years ago

Closing in favor of #72