Code review comment for lp:~daniel-nichter/drizzle/fix-slave-reconnect-bug-956200

Dave,

I agree: we need additional states, and I had already planned to tackle that too after further discussion. For right now, however, slave is broken and this patch is just a first step to fix it. More work is needed, but who knows when that work will be done. Since the patch doesn't break or regress any features, I think it's worth accepting now, and we'll continue to improve it when we can.

Le 1 mai 2012 à 10:23, David Shrewsbury a écrit :

> Review: Disapprove
>
> Although I whole-heartedly agree with the changes you are trying to make, I think parts of it need reworked a bit.
>
> We need to keep the concept of when a thread is actually alive (i.e., RUNNING) vs. when the thread has terminated (STOPPED). You've essentially redefined those states here, and that's the part I disagree with. What we need instead is to add additional thread states. E.g.,
>
> * running and initializing (INITIALIZING). This would likely replace RUNNING.
> * running and disconnected and will not attempt reconnect (DISCONNECTED)
> * running and awaiting reconnect (RECONNECTING)
> * running and connected normally (CONNECTED)
> * running but in some awful error state (ERROR)
> * thread has terminated normally (STOPPED)
> * etc.
>
> Something along those lines. Those states are just off the top of my head and I'm open to what is defined.
>
> And although I was the one to move max-reconnects to each [masterN] section for some inexplicable reason, wouldn't we want the max-reconnect, seconds-between-reconnects, and io-thread-sleep values to be common values among all defined masters, rather than defined separately for each master?
> --
> https://code.launchpad.net/~daniel-nichter/drizzle/fix-slave-reconnect-bug-956200/+merge/104046
> You are the owner of lp:~daniel-nichter/drizzle/fix-slave-reconnect-bug-956200.