doxout / recluster

Node clustering library with support for zero downtime reloading
522 stars 44 forks source link

WIP - rework singleton logic #30

Closed Sinewyk closed 9 years ago

Sinewyk commented 9 years ago

Hmm ...

It's a lot easier to reason about if recluster exports a singleton, just like the cluster module. assert uses for state conformity.

Tests are fully isolated (you will be happy that I came back to tap :stuck_out_tongue_closed_eyes: ), and we have some hooks for some events, listening when at least one working is listening, fully-listening when all workers are listening, stopped when we asked cluster to stop and all workers exit-ed.

My objective is to get to where you were before in terms of feature, and get no downtime in case of proper reload (should be working if cluster.workers aren't lying about their state).

This should be a major version increase in case it is merged.

And as always, I'm fully open to dialogue/help :smile:

Missing:

There's still some Error: channel closed sometimes, I still don't quite know if it's my fault or because cluster module is still unstable.

spion commented 9 years ago

Hate to be a party breaker, but why not publish this as a completely different module? :D The changes are too big to introduce to recluster without risking breakage and stability for existing users (including my old company doxout), and I'm not sure if I'll be able to find the time to throughly review them...

I may steal your tests though, so be forewarned ;)

(And by too big, I do mean that every single line has been replaced, lol)

Sinewyk commented 9 years ago

Haha, yeah, it's a rewrite.

I do strongly advise you to at least try to export a singleton instead of exporting a kinda class constructor. It makes everything easier imo.

Steal away bro ^^. I'll probably just use this internally then.

Cya =).

Sinewyk commented 9 years ago

Oh yeah. I forgot also, when using the reloading, you should try to spawn new workers maybe before shutting down the old ones. Like ... to ensure 0 downtime maybe ?

It's what I tried to implement and it 'seems' to be working https://github.com/doxout/recluster/pull/30/files#diff-168726dbe96b3ce427e7fedce31bb0bcR167, if we believe the unit tests https://github.com/doxout/recluster/pull/30/files#diff-03cedc2df2fa9d01239c4790a0e7d0deR89

PS: I don't know what you workflow is ... but seriously ... nodemon ./node_modules/.bin/tap test/*.js is a must for TDD, you just code and immediate feedback. It's the first time I seriously used TDD, and I like it a lot :stuck_out_tongue:

spion commented 9 years ago

recluster does spawn new workers before shutting down old ones. The stopOld function is wrapped by allReady, which ensures that at least the same number of workers have been spawned and have become ready before shutting down the old ones.

Sinewyk commented 9 years ago

Hmm. Weird, in practice it didn't seem to work, I always had a (very small, sub second) downtime.