in 3.4 version of this patch, we have an if statement to ignore UPDATE_ACK ((zxid & 0xffffffffL) == 0) in processAck(). Looking at the code, the learner from both 3.4 and 3.5 still sending UPTODATE_ACK, so I think we still need to have that similar logic unless it is needed by reconfiguration.
We still have that if statement. The one I was talking about previously that has been removed is in Leader#tryToCommit().
acking a NEWLEADER message means that state transfer is complete, Observer or not, so why not count this guy's vote. On the other hand, he is not yet a participant, and in the normal (non-recovery) case we wouldn't count its vote (he wouldn't send any).
In the case you describe, I thought that the observer (changing to participant) should actually be able to vote because we also need a quorum for the new config, which may also come in a newleader message, no? If this is correct, then we shouldn't have the if statement.
I think that (zxid & 0xffffffffL) == 0 means its a NEWLEADER ack, not UPTODATE ack.
You're both right. We send a second ack after the uptodate, and it uses the same zxid. It is there for backward compatibility reasons, the ack itself is strictly necessary for the correctness of the protocol.
Once we decide what to do with the if statement Alex referred to, I'll make any necessary changes and upload a new patch.