ZOOKEEPER-4220: Potential redundant connection attempts during leader election

[symat]

We have a logic in the server code, that would try to connect to an other quorum member, based
on its server ID. We identify the address assigned to this ID first based on the last committed
quorum configuration. If the connection attempt fails (or the server is not known in the
committed configuration) then we try to find the address based on the last proposed quorum
configuration. But we should do the second connection attempt, only if the address in the
last proposed configuration differs from the address in the last committed configuration.
Otherwise we would just retry to connect to the same address that failed just right before.

In the current code we have a bug, because we compare the address object references (use "!=")
instead of comparing the objects themselves (using "not equals"). In certain edge cases (e.g.
when the last proposed and last committed addresses are the same, but the address is unreachable)
this bug can lead to unnecessary retry of connection attempts. The normal behaviour would be to
mark this connection attempt to be failed and wait for e.g. the next election round or wait for
the other server to come online and initiate a connection to us.

[anmolnar]

+1
Very nice catch @symat !
Do you think you can write a unit test for this bit?

[symat]

Very nice catch @symat !

it was actually Alex Mirgorodskiy (see the jira issue), the credits go to him ;)

Do you think you can write a unit test for this bit?

good idea, I'm thinking about it. I already created a related unit test not much long ago. QuorumCnxManagerSocketConnectionTimeoutTest. I think this could be extended. Let me give a try.

[symat]

unfortunately it is not really possible to create any clean and non-flaky unit test. The problem is with the async connection initiation, which makes the problem hard to reproduce. Since https://issues.apache.org/jira/browse/ZOOKEEPER-3756, we are always initiating leader election connections asynchronously. Before submitting the new connection initiation thread to the executor, we check if already is a thread submitted for the given address. Depending on the scheduling of the JVM / CPU, we may or may not submit the redundant connection attempt we try to fix here.

We could introduce some configurable (only-visible-for-tests) sleep inside the QuorumCnxManager to a certain point making sure we indeed hit this problem. But I'm not favour of complicating the production code this way.

I spent a few hours to make a nice test, but now I kind of gave up. I think this is a trivial fix, I can live without testing this edge case. What do you think?

[symat]

@anmolnar what do you think? can I merge this without unit test?

I plan to merge it to all active branches.
@ztzg can this still fit into 3.7.0? should I also push this to branch-3.7.0, or branch-3.7 is enough for that?

[ztzg]

LGTM.

Re testing: agree that incredibly convoluted tests are not always value-added.

(Interesting that MultipleAddresses.equals does not consider the timeout field, btw. Not that I expect it to have any impact here.)

[ztzg]

Hi @symat, @anmolnar,

I have merged this in master, branch-3.7, and branch-3.7.0. I hope Andor won't mind, and I would gladly add a test later if we come up with a reasonable idea. I have not merged it in 3.5 nor 3.6 so far, as I prefer to wait for Andor's definitive answer when it comes to these "very stable" branches. What do you think?

[ztzg]

@symat, @anmolnar : Just making sure you have spotted this:

I have not merged it in 3.5 nor 3.6 so far, as I prefer to wait for Andor's definitive answer [wrt. including tests] when it comes to these "very stable" branches. What do you think?

[symat]

sure, thanks @ztzg !
I'm happy to do the cherry-picks to the stable branches (and run some smoke tests), if it's OK for Andor.

[anmolnar]

@ztzg I don't have a strong opinion on unit tests. I think it's fine as it is.
This should be merged to all active branches. Please go ahead.

[symat]

I created #1630 and #1631 to start CI on the 3.5 and 3.6 branches.