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

Revision history for this message
David Shrewsbury (dshrews) wrote :

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?

review: Disapprove

« Back to merge proposal