[Fix] Curator-444

[borjab]

This ensures that not two nodes are simultaneusly as leaders.

If setLeadership(false) is redundant no event would be reported anyway so it is quite safe.

[borjab]

Fix now includes test to reproduce error.

[borjab]

Thanks for the feedback. You are right that the test has a lot of room for improvement. I will work on the tests as requested.

[cammckenzie]

Thanks!

[egor-ryashin]

We've got continuous issues with that bug (two critical nodes becoming leaders simultaneously begin to damage the work of each other), and I would like this work to be continued, though I see it lost attention some time ago. Right now I'm looking at the code trying to figure out where exactly it's broken and I found this line quite strange https://github.com/apache/curator/pull/243/files#diff-edf135a1ae8f33bd78d8371b25f2d5a6R544 IE, we got 3 children:

0 1 2

And ourIndex = 2, why would we add a watcher for the child 1?

sortedChildren.get(ourIndex - 1)

[cammckenzie]

This is just done for efficiency. Each client involved in the leader election only cares when the child before it disconnects, because that implies that it should now become leader. If any of the other children disconnect, the client doesn't care because it doesn't affect whether it should become leader or not.

[Randgalt]

And ourIndex = 2, why would we add a watcher for the child 1?

This prevents thundering herds when ZNodes are deleted. A given client only needs to watch the ZNode previous to it thereby it only gets notified when it will be the next lock/leader holder.

[Randgalt]

@borjab and @egor-ryashin I just ran the test in this PR multiple times with the fix in LeaderLatch commented out and the tests always pass. It's important to note that if you don't use the default ConnectionStateErrorPolicy and/or elsewhere don't exit your locks, etc. when you receive SUSPENDED you can get in this state. Even then with long GC times there is always the possibility of multiple lock/leader holders. See https://cwiki.apache.org/confluence/display/CURATOR/TN10

[egor-ryashin]

@Randgalt I've actually seen 2 leaders working several minutes till I restart them. ~My guess, right now, that can happen when the leader has lost connection for a spell. For example, for children: A B here B watches A, while A doesn't watch anything, I guess. If A gets a connectivity issue, B becomes a leader and set no watches, then A get through and becomes the leader again, due to the sorting rule, while B doesn't receive any notification about it. Haven't had the time to prove it yet though.~ (More thourough looking through code and some tests shows it's not the root cause.)

[Randgalt]

@egor-ryashin LeaderLatch calls setLeadership(false); when the connection is lost. LeaderSelector is a bit more complicated but if you use the recommended LeaderSelectorListenerAdapter the active leader will get interrupted when the connection is lost which will result in a release of the its internal lock. In both cases the lock node (A in your example) gets deleted on connection loss.

[Randgalt]

Please read this tech note and see if it applies: https://cwiki.apache.org/confluence/display/CURATOR/TN14

[egor-ryashin]

@Randgalt no, it doesn't recover, only a restart helps.

[tisonkun]

Closed as stale. There're many changes since this patch was made in the first place. Feel free to resubmit it if it's still relevant.